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

Remove ENABLE_MATH option from pulldown_cmark to fix links which contain dollar sign #22647

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

spotikhanov
Copy link
Contributor

This pr closes #21466 issue by disabling math in pulldown_cmark Parser.

The dollar sign symbol is used in pulldown_cmark Math extension, see "Math in links" section for more details: https://pulldown-cmark.github.io/pulldown-cmark/specs/math.html

I've tried another approach at first, without disabling math extension:

let iterator = TextMergeWithOffset::new(Parser::new_ext(text, options));

instead of current implementation

Parser::new_ext(text, options).into_offset_iter()

This way pulldown_cmark merges consecutive text events and this helps to correctly parse links from plain text: https://svelte.dev/docs/svelte/$state

But in this case the dollar sign still breaks markdown links: [https://svelte.dev/docs/svelte/$state](https://svelte.dev/docs/svelte/$state)

So in the end I disabled the math extension, it fixes both link formats. See markdown/examples/markdown.rs to reproduce.

Release Notes:

  • N/A

Copy link

cla-bot bot commented Jan 4, 2025

We require contributors to sign our Contributor License Agreement, and we don't have @spotikhanov on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

@spotikhanov
Copy link
Contributor Author

@cla-bot check

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jan 4, 2025
Copy link

cla-bot bot commented Jan 4, 2025

The cla-bot has been summoned, and re-checked this pull request!

@zed-industries-bot
Copy link

Warnings
⚠️

This PR is missing release notes.

Please add a "Release Notes" section that describes the change:

Release Notes:

- Added/Fixed/Improved ...

If your change is not user-facing, you can use "N/A" for the entry:

Release Notes:

- N/A

Generated by 🚫 dangerJS against 724b539

@cole-miller
Copy link
Contributor

Makes sense to me since we don't handle the Math events from pulldown_cmark at all. Thanks!

@cole-miller cole-miller added this pull request to the merge queue Jan 7, 2025
Merged via the queue into zed-industries:main with commit a653e8a Jan 7, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Links in JSDoc popovers stop at dollar sign ($)
3 participants