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

Update pulldown-cmark dependency to 0.10 #67

Merged
merged 16 commits into from
Mar 16, 2024

Conversation

max-heller
Copy link
Contributor

@max-heller max-heller commented Feb 10, 2024

Version 0.10 of pulldown-cmark was just released with a number of breaking changes. Unfortunately, it doesn't seem straightforward to update pulldown-cmark-to-cmark's dependency on it. This is a start, but there are some breaking changes that will have to be worked around (compilation currently errors)

@Byron Byron marked this pull request as draft February 11, 2024 06:26
@liningpan
Copy link

liningpan commented Feb 23, 2024

Do you have any plans for fixing those breaking changes? It seems like upstream removed some information from the end tags to save some memory allocations pulldown-cmark/pulldown-cmark#517. I suppose it means that we have to push start tags onto a stack as our internal state.

@max-heller
Copy link
Contributor Author

Do you have any plans for fixing those breaking changes? It seems like upstream removed some information from the end tags to save some memory allocations pulldown-cmark/pulldown-cmark#517. I suppose it means that we have to push start tags onto a stack as our internal state.

pushed a commit that introduces some additional stacks to keep track of links/images, and the current heading. need to fix compilation of the tests to see if it works

@max-heller
Copy link
Contributor Author

updated most of the tests but tests/display.rs is tricky because it attempts to format end tags on their own, which now do not contain the necessary information

@TerakomariGandesblood
Copy link

Any updates here?

@max-heller
Copy link
Contributor Author

Any updates here?

Mainly not sure how to update some of the tests, @Byron do you have any ideas for the end tag tests?

@Byron
Copy link
Owner

Byron commented Mar 15, 2024

It looks like only a single test is failing, which lets me hope this PR is close to merging actually.

updated most of the tests but tests/display.rs is tricky because it attempts to format end tags on their own, which now do not contain the necessary information

Is the intent of the failing test clear? If it is, can it be adjusted to deal with the changes made here? Could the same thing be tested differently?
If the intent is not clear, it could possibly be removed and replaced with something that makes more sense. I still hope that the test is protecting something valuable though, and that it can be salvaged.

@max-heller
Copy link
Contributor Author

Adjusted the end tag display tests and fixed the failing test, this needs some cleanup to avoid tons of extra allocations but it's passing the tests!

@max-heller max-heller marked this pull request as ready for review March 16, 2024 00:33
@max-heller
Copy link
Contributor Author

max-heller commented Mar 16, 2024

Is there a reason State isn't allowed to borrow from events? It's constrained to State<'static> in many APIs, but this doesn't seem necessary, and we could avoid allocating a ton of strings if we let it borrow. Taking in Iterator<Borrow<Event>> is also somewhat restrictive compared to Iterator<Event>, since we can't reuse owned strings from events

@max-heller max-heller changed the title [WIP] Update pulldown-cmark dependency to 0.10 Update pulldown-cmark dependency to 0.10 Mar 16, 2024
@Byron Byron merged commit 640148b into Byron:main Mar 16, 2024
1 check passed
Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot, great work!

The spec-tests improved conformity by 4 as well, and I appreciated the added test to check support for ids and attributes as well.

A new release is now available: https://github.com/Byron/pulldown-cmark-to-cmark/releases/tag/v12.0.0

@max-heller max-heller deleted the update-pulldown-cmark branch March 16, 2024 10:47
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.

4 participants