Skip to content

[Bugfix] Fix the backticks bug #26

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

Merged
merged 2 commits into from
Nov 10, 2021
Merged

Conversation

Kyle-Ye
Copy link

@Kyle-Ye Kyle-Ye commented Nov 7, 2021

@Kyle-Ye
Copy link
Author

Kyle-Ye commented Nov 7, 2021

I do not know this will break any other situation. But this did address the issue of wrongly parse backticks mentioned in the SR-15415.

When parse "`" and "``"
Before the PR: We'll get 1:2-1:2 and 1:3-1:3 for the Text Node.
After the PR: We'll get the intended 1:1-1:1 and 1:1-1:2 for the Text Node

@QuietMisdreavus
Copy link

Can you make sure to run swift run api_test locally to run cmark-gfm's built-in tests? You could also add something to the source_pos() test in api_test/main.c to make sure your fix works even outside swift-markdown. Otherwise, this looks great, thanks!

@akyrtzi
Copy link

akyrtzi commented Nov 10, 2021

Assuming this is code from upstream cmark, could we fix it in upstream and then sync-up the fork instead?

@Kyle-Ye
Copy link
Author

Kyle-Ye commented Nov 10, 2021

Can you make sure to run swift run api_test locally to run cmark-gfm's built-in tests? You could also add something to the source_pos() test in api_test/main.c to make sure your fix works even outside swift-markdown. Otherwise, this looks great, thanks!

It can pass the test by running swift run api_test
Screen Shot 2021-11-10 at 13 09 25
And added test to source_pos_inlines() in api_test/main.c

@Kyle-Ye
Copy link
Author

Kyle-Ye commented Nov 10, 2021

Assuming this is code from upstream cmark, could we fix it in upstream and then sync-up the fork instead?

Add a clean PR to the upstream. See commonmark#427

Copy link

@QuietMisdreavus QuietMisdreavus left a comment

Choose a reason for hiding this comment

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

Thanks so much for adding the test, and for posting the patch upstream! It looks like they've already taken a look, which is nice. This looks good to go for me, now.

@akyrtzi
Copy link

akyrtzi commented Nov 10, 2021

Assuming this is code from upstream cmark, could we fix it in upstream and then sync-up the fork instead?

Add a clean PR to the upstream. See commonmark#427

Thank you! 🙇🏻‍♂️

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.

3 participants