-
-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Add Graph property support #182
base: main
Are you sure you want to change the base?
Conversation
This next part is kinda a ramble of thoughts about this all:
One problem ends up being though is that people may expect Let's think deserialization:
Now serialization:
|
We need to add some documentation to the README. It might be a good idea to also link to some documentation about I suspect we also need to protect against using |
Hey all, I was thinking of taking a stab, or trying to help, with this feature, and it looks like some maintainer-level decisions are still being worked through on how best to handle it. You all may be waiting on the System.Text.Json move as well, which would make sense. In case some feedback from a users's perspective (typically a scraping use case without much known about the json upfront) is helpful:
Happy to help once a direction is settled on. And thanks for your work on this library, it's nice. |
Thanks for commenting @NickSpag - I'm happy for you to take a shot at this issue.
So the reason I think it is a bit crappy is that you may not know that the JSON-LD is a graph or not when deserializing. If you called We may need to have
I think if we can prevent |
Ah, okay I see your thinking there- I had mentally separated the two user paths in to known graph or known object. One question to help my understanding, are there other scenarios in the library where an array of multiple different types is expected? If so, how are they handled? I know there's the But to expand on your first attempt @Turnerj at mapping scenarios, it looks like there are: Deserialize
SerializeThis entire direction seems more straightforward since we already know the types. Given the serialization path goes through ApproachesCould include one, or a combo of, the following:
With an open question relating to the above: do any of those approaches also need an interface change incorporated, per your comments about the users's concept of a root interface/abstraction How's that sound so far? Thoughts? |
I've re-written this comment multiple times before posting it as throughout writing it, I realised various different things that made my previously written thoughts obsolete or incomplete. What you see below is what survived the cutting room floor of my mind...
It really depends how we think about how we deserialize. We could use I wrote a whole big thing how I think the main issue is about method signatures, specifically Perhaps we create a new We could additionally have a So let's assume
✅ If the JSON graph was of a
❓ If the JSON graph was of a One thing we could do is have the
✅ If the JSON graph was of a
❓ Similar to the second point, if the JSON graph was of a
✅ In this scenario with
Yeah, serialization is wildly easier no matter how we go about it. In this scenario with a
Hey - you had the same thought as me - it really pays for me to keep re-reading as I keep re-writing.
Now that I've thought through the idea of a Whether we go super fancy and have generics for In terms of specific interfaces needing changing, I don't think they would need to. If we were to do one of the other approaches with Hopefully I've kinda answered/addressed your points! |
ae249b4
to
4c5521e
Compare
Getting the ball rolling for
@graph
support. Will close #132 when merged.Adding the property is easy - working out where to put the logic about conditionally serializing
@graph
over other properties or how to deserialize it (as the deserialization target "should" be aJsonLdObject
, not any type inheriting it) will be tricky. Perhaps can only deserialize a@graph
if you chooseJsonLdObject
(eg.SchemaSerializer.DeserializeObject<JsonLdObject>(myJson)
) though that seems a bit crappy.I've marked this as
minor
currently as in theory, this should be backwards compatible. That said, happy to have this marked asmajor
instead.