Skip to content

[Bugfix] Fix the backticks bug#26

Merged
QuietMisdreavus merged 2 commits intoswiftlang:gfmfrom
Kyle-Ye:gfm
Nov 10, 2021
Merged

[Bugfix] Fix the backticks bug#26
QuietMisdreavus merged 2 commits intoswiftlang:gfmfrom
Kyle-Ye:gfm

Conversation

@Kyle-Ye
Copy link
Copy Markdown

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

@Kyle-Ye
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

@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
Copy Markdown

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