-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add optional serde support #63
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, the JSON output in the web demo came in handy already.
There is some awkward resulting JSON since the types
were not really designed with this direct serialization in mind. I am
not sure how much we want to massage the output as not to stray away too
far from the actual types.
-
The Attributes object is quite representative of the original source,
e.g{a=b .b .b}
yields
[ [ { "Pair": { "key": "a" } }, "b" ], [ "Class", "b" ], [ "Class", "c" ] ]
It would be possible to instead use the output from
Attributes::unique_pairs which should instead yield:{ "a": "b", "class": "b c" }
I'm guessing that would be more convenient to work with. You would
lose the ability to use comments, though.The deserialization would also need some manual implementation. I
guess e.g.{"class": "a b c"}
could be turned into{.a .b .c}
and{"id": "id"}
into{.id}
while all remaining keys would just
become Pair variants. -
Tuples are kind of awkward in JSON:
-
Start:
"Start": [ "Paragraph", [] ]
would probably be better represented with
"Start": { "container": "Paragraph", "attributes": [] }
-
Link/Image:
"Link": [ "https://example.com", { "Span": "Inline" } ]
could be
"Link": { "url": "https://example.com", "type": { "Span": "Inline" } }
-
ThematicBreak:
[ { "ThematicBreak": [] } ]
It is not obvious that the empty list is attributes, so it
could be[ { "ThematicBreak": { "attributes": [] } } ]
-
-
There is a lot of nesting in general, it might be possible to do some
flattening.-
Some nested enums could maybe be flattened, e.g.
"Link": [ "url", { "Span": "Inline" } ]
could be just
"Link": [ "url", "Inline" ]
Does not work for ListKind though, as two of the variants use the
ListBulletType enum. -
Have you considered using internally tagged enums?
Although, not sure if it is possible to specify the names
for tuple variant elements? Some could potentially benefit from
being converted to structs, though.E.g. lists could benefit from internal tagging to reduce the nesting:
"List": { "kind": { "Ordered": { "numbering": "Decimal", "style": "Period", "start": 1 } }, "tight": true }
could instead be
"List": { "kind": { "type": "Ordered" "numbering": "Decimal", "style": "Period", "start": 1 }, "tight": true }
and with flattening of kind:
"List": { "type": "Ordered" "numbering": "Decimal", "style": "Period", "start": 1 "tight": true }
-
-
Do we want to use lowercase names for all types? The PascalCase looks
a little bit odd in JSON. On the other hand, maybe lowercase looks
weird in other serialized formats?
Again, not sure how much pre/post-processing we want to perform, just
pointing out some things that seem potentially awkward. You may
have a better idea what would actually improve the experience for your
use case at least.
Also, if something takes too much effort to implement, as-is still
better than nothing. I might also be to help out if we both agree that
something would be an improvement.
Also, could spans be useful to have in the JSON output? I guess it would Currently simply [
{
"Start": [
"Paragraph",
[]
]
},
{
"Str": "a"
},
{
"End": "Paragraph"
}
] With internal tagging it could be: [
{
"type": "Start":
"container": {
"type": "Paragraph"
},
"attributes": []
},
{
"type": "Str",
"content": "a"
},
{
"type": "End",
"container": {
"type": "Paragraph"
}
}
] For [
{
"type": "Start":
"container": {
"type": "Paragraph"
},
"attributes": []
"src_start": 0,
"src_end": 0,
},
{
"type": "Str",
"content": "a"
"src_start": 0,
"src_end": 1,
},
{
"type": "End",
"container": {
"type": "Paragraph"
}
"src_start": 1,
"src_end": 1,
}
] |
Thanks for your detailed feedback. I'm sorry that I didn't respond. For the project that I was needing the events for I have realized that I need more fine-grained events anyway (#78). djot.js currently works fine for me there but I now have again another project where I'm again interested in using jotdown (this time I'm interested in an AST though). Regarding:
I just wanted to note that that's impossible in Rust since you cannot implement a foreign trait for a foreign type and tuples are always foreign. |
Resolves #61.