-
Notifications
You must be signed in to change notification settings - Fork 32
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
🐛 FIX: Task list item marker can be followed by any GFM whitespace #42
🐛 FIX: Task list item marker can be followed by any GFM whitespace #42
Conversation
Codecov ReportBase: 92.36% // Head: 93.20% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #42 +/- ##
==========================================
+ Coverage 92.36% 93.20% +0.83%
==========================================
Files 27 25 -2
Lines 1572 1353 -219
==========================================
- Hits 1452 1261 -191
+ Misses 120 92 -28
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
or token.content.startswith("[x] ") | ||
or token.content.startswith("[X] ") | ||
) | ||
return re.match(r"\[[ xX]][ \t\n\v\f\r]+", token.content) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use \s
here, to match whitespace? (see https://docs.python.org/3/library/re.html)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do that, but then have to set the re.ASCII
flag, else we match too much. I listed all the chars so that they are easy check the against the GFM spec which does so too: https://github.github.com/gfm/#whitespace-character
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CommonMark specification (because that is the reference here!) does the same: https://spec.commonmark.org/0.29/#whitespace-character .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😂 can you a link to this white space definition in a comment or the docstring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest CM spec is 0.30 and that doesn't have the same character set. The latest spec only has "unicode whitespace".
I would recommend only referring to the latest GFM spec here, since task lists have nothing to do with CM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you a link to this white space definition in a comment or the docstring
Sure. I think I'll make it a private variable or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the new CM spec is not fully updated then - it still refers to whitespace in the examples without defining it: https://spec.commonmark.org/0.30/#example-350 .
I was actually thinking why this should not defined as a constant because it has just proven to change over time... . GFM_WHITESPACE, CM_WHITESPACE.
Edit: which you did 😃
Closes #41
This issue was discovered by @mdeweerd in hukkin/mdformat-gfm#23
We should probably upstream this to https://github.com/revin/markdown-it-task-lists if it's still maintained.