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
Fixes ordered list ")" delimiter #1704
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/markedjs/markedjs/6vqu1a1zs |
Can you write a test to make sure no one breaks it in the future. |
The tests have been included. |
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.
Great work! just a few changes.
test/unit/Lexer-spec.js
Outdated
@@ -345,19 +345,27 @@ a | b | |||
md: ` | |||
1. item 1 | |||
2. item 2 | |||
3) item 3 | |||
4) item 4 |
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.
This unit test actually goes against the spec example 272. marked should start a new list if the bullet or ordered list delimiter is changed.
I'm ok with not fixing that spec example in this PR but can you create an integration test instead of changing the unit test. You can create a new .md
file and .html
file in /test/specs/new/
something like:
list_paren_delimiter.md
1) one
2) two
3) three
2) two
3) three
4) four
list_paren_delimiter.html
<ol>
<li>one</li>
<li>two</li>
<li>three</li>
</ol>
<ol start="2">
<li>two</li>
<li>three</li>
<li>four</li>
</ol>
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.
I've made changes to accommodate ex.272. I am not too sure what the shouldFail
keyword means in the CM tests, but some of them had to be removed. In any case, the fixes work well.
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 shouldFail
is there so we can track which spec tests are passing and which are failing.
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.
So shouldFail
means shouldNotFailButDoesFail
?
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.
exactly 😁👍
Co-authored-by: Tony Brix <tony@brix.ninja>
Co-authored-by: Tony Brix <tony@brix.ninja>
We require 2 Approvals for PRs so this will be merged when another team member approves it. |
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.
Thanks!
Marked version: Latest
Markdown flavor: CommonMark
Description
Contributor