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

Sync with cmark-gfm-0.29.0.gfm.11 #319

Merged
merged 4 commits into from
Jun 18, 2023

Conversation

digitalmoksha
Copy link
Collaborator

This syncs with cmark-gfm-0.29.0.gfm.11 release

@digitalmoksha digitalmoksha marked this pull request as draft June 14, 2023 23:34
@digitalmoksha
Copy link
Collaborator Author

digitalmoksha commented Jun 14, 2023

@kivikakk I haven't updated fuzz/quadratic.rs or created fuzz/quadratic_brackets.rs yet.

I also have a small doubt on render.c in github/cmark-gfm@0.29.0.gfm.10...0.29.0.gfm.11. I don't think we need it, or at least I can't find where we would incorporate the changes 🤔

@digitalmoksha
Copy link
Collaborator Author

Woohoo, green pipeline!

@digitalmoksha
Copy link
Collaborator Author

So I think I've made the necessary changes to fuzz/quadratic.rs.

Looking at how you're using Markdown::Markdown, Markdown::Sandwich, and Markdown::Tree, I'm guessing we would want a Markdown::Brackets, rather than creating an entirely separate quadratic_brackets.rs file.

Though I admit my eyes are glazing over right now, and I'm not sure how it should be coded. I'm quite fuzzy on the fuzzer 😬

@kivikakk
Copy link
Owner

Amazing! Thank you so much!

Looking at how you're using Markdown::Markdown, Markdown::Sandwich, and Markdown::Tree, I'm guessing we would want a Markdown::Brackets, rather than creating an entirely separate quadratic_brackets.rs file.

I'm not sure myself—I didn't write it either. It looks to me like Markdown::Sandwich might actually be that? fuzz_quadratic_brackets.c seems to do something very similar—there's a section repeated some number of times before and after.

I also have a small doubt on render.c in github/[email protected]. I don't think we need it, or at least I can't find where we would incorporate the changes 🤔

This is actually making it clear that there's another difference between Comrak and cmark-gfm, perhaps since that change. See:

$ printf '3. x\n5. z' | cargo run -- --to commonmark
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
     Running `target/debug/comrak --to commonmark`
3.  x
5.  z

$ printf '3. x\n5. z' | build/src/cmark-gfm -t commonmark
3.  x
4.  z

This difference isn't really able to be captured in the spec, since it only checks HTML output. You could consider Comrak's roundtripping here more information-preserving, and it's not clear that our approach is worse.

You've done really well. Thank you. I'm happy to merge this now if you are.

@kivikakk
Copy link
Owner

(Also, don't worry about the conflict, I'll resolve it when I merge.)

@digitalmoksha
Copy link
Collaborator Author

You could consider Comrak's roundtripping here more information-preserving, and it's not clear that our approach is worse.

Hmm, you're right. But I think the main reason that I would use the commonmark output option would be to cleanup some text. I think in that case it would be preferable to have the items renumbered.

But it certainly could be looked at either way. I went ahead and opened #323, because I found a problem with the numbering when using task lists. Neither of these are a big deal since the resulting commonmark is valid.

It looks to me like Markdown::Sandwich might actually be that?

I think you're probably right. 😌

I'm happy to merge this now if you are.

Let's do it! 🚀

@digitalmoksha digitalmoksha marked this pull request as ready for review June 17, 2023 16:42
@kivikakk kivikakk enabled auto-merge June 18, 2023 02:10
@kivikakk kivikakk merged commit 544f5a3 into kivikakk:main Jun 18, 2023
@kivikakk
Copy link
Owner

🎉 🎉 🎉 Amazing! Thank you so much! We're finally CI green again! 🍏

@digitalmoksha
Copy link
Collaborator Author

Hey thanks for working through these with me so fast!

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.

2 participants