Skip to content
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

Fix serde, change BlockQuote -> Blockquote #135

Merged
merged 4 commits into from
Sep 17, 2024
Merged

Conversation

h7kanna
Copy link
Contributor

@h7kanna h7kanna commented Sep 15, 2024

NOTE : The included sanity test makes sure that at-least the AST serializes and deserializes. More rigorous tests should be added later. Currently, it works as the construct names 'camelCased' match the Mdast JSON except for Blockquote (changing it will be API change).
As it is still in alpha changing 'BlockQuote' enum to 'Blockquote' will make serde work completely.

NOTE: This PR makes another pending one obsolete #74

@@ -264,6 +264,9 @@ The following bash scripts are useful when working on this project:
```sh
RUST_BACKTRACE=1 cargo test
```
```sh
RUST_BACKTRACE=1 cargo test --features json
```
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you recommend that humans run both commands? One or the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be replaced with 'cargo test --all-features'

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, can you do that? One test example, with all-features?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@wooorm
Copy link
Owner

wooorm commented Sep 16, 2024

Thanks for your PR!


(changing it will be API change).
As it is still in alpha changing 'BlockQuote' enum to 'Blockquote' will make serde work completely.

Are you saying that currently (main branch) the type field is not blockquote (the correct value according to mdast)?
And that this branch doesn’t change that either?
I would prefer to fix it: to use blockquote.


Are you interested in adding, as you note in your OP, some more tests?

@h7kanna
Copy link
Contributor Author

h7kanna commented Sep 16, 2024

Thanks for your PR!

(changing it will be API change).
As it is still in alpha changing 'BlockQuote' enum to 'Blockquote' will make serde work completely.

Are you saying that currently (main branch) the type field is not blockquote (the correct value according to mdast)? And that this branch doesn’t change that either? I would prefer to fix it: to use blockquote.

No, in the main branch it is 'blockquote', and matches the mdast. I had to remove the 'type' on the individual enums and replace with 'rename_all=camelCase' on the 'Node'. It seems like we cannot correctly specify tagging on individual constructs. It is resulting in duplicate tags like shown below. I do not like the solution of depending the convention of the camelCase. But I am assuming it will never change.

{
         "type":"Blockquote",
         "type":"blockquote",

Are you interested in adding, as you note in your OP, some more tests?

Yes I started writing tests. But they are turning really verbose. I am refactoring them.
But I created the PR to ask about the 'Blockquote' change and the assumption of 'camelCase' convention.
Also this PR can be individually merged because currently enabling 'serde' feature is not usable at all because of invalid json output.

@wooorm
Copy link
Owner

wooorm commented Sep 16, 2024

invalid json

I don’t think JSON has a limitation on non-duplicate fields? https://www.json.org/json-en.html.

individually merged

fine!


Changing Block Quote is 👍 for me!

@h7kanna
Copy link
Contributor Author

h7kanna commented Sep 16, 2024

invalid json

I mean deserialization failing because of above duplicate 'type'.

I don’t think JSON has a limitation on non-duplicate fields? https://www.json.org/json-en.html.

individually merged

fine!

Changing Block Quote is 👍 for me!

Yes, made the change as well.

@wooorm
Copy link
Owner

wooorm commented Sep 17, 2024

I mean deserialization failing because of above duplicate 'type'.

Right, but I am trying to point out that I do not think this is not something JSON defines. I think it is something that some libraries do: in JS, JSON.parse('{"a":1,"a":2}') is {a:2}.

Copy link
Owner

@wooorm wooorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Are you planning to add some more tests in this PR, or separately?

@h7kanna
Copy link
Contributor Author

h7kanna commented Sep 17, 2024

Looks good. Are you planning to add some more tests in this PR, or separately?

Definitely adding more test cases. I will do in follow up PR. It is just taking time to write down the fixtures. Most of the content is just spans and points.

@wooorm wooorm changed the title Fixed mdast serde Fix serde, change BlockQuote -> Blockquote Sep 17, 2024
@wooorm wooorm merged commit fa8f906 into wooorm:main Sep 17, 2024
3 checks passed
@wooorm
Copy link
Owner

wooorm commented Sep 17, 2024

Thanks!

@wooorm
Copy link
Owner

wooorm commented Sep 17, 2024

Looking forward to your other PR!

@h7kanna h7kanna deleted the fix-serde branch September 17, 2024 13:28
This was referenced Sep 17, 2024
wooorm pushed a commit that referenced this pull request Sep 20, 2024
Related-to GH-135.
Related-to GH-137.
Closes GH-140.
wooorm added a commit that referenced this pull request Sep 20, 2024
Related-to: GH-135.
Related-to: fa8f906.
wooorm added a commit that referenced this pull request Sep 20, 2024
@wooorm
Copy link
Owner

wooorm commented Sep 20, 2024

@h7kanna Do you perhaps have an idea how to fix this?

markdown-rs/src/mdast.rs

Lines 81 to 82 in 4ad5342

// To do: this should serialize in serde as `null`.
None,

It should serialize in JSON as null. Not "none".

zannabianca1997 added a commit to zannabianca1997/mdast2minimad that referenced this pull request Oct 8, 2024
Fixed a bug introduced with <wooorm/markdown-rs#135> renaming a enum variant.
yshavit added a commit to yshavit/mdq that referenced this pull request Dec 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MDAST: serde Serialization/Deserialization not working
2 participants