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

Fixes ordered list ")" delimiter #1704

Merged
merged 12 commits into from Jun 17, 2020
Merged

Fixes ordered list ")" delimiter #1704

merged 12 commits into from Jun 17, 2020

Conversation

jun-sheaf
Copy link
Contributor

@jun-sheaf jun-sheaf commented Jun 12, 2020

Marked version: Latest

Markdown flavor: CommonMark

Description

Contributor

  • Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
  • no tests required for this PR.
  • If submitting new feature, it has been documented in the appropriate places.

@vercel
Copy link

vercel bot commented Jun 12, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/markedjs/markedjs/6vqu1a1zs
✅ Preview: https://markedjs-git-fork-jun-sheaf-master.markedjs.vercel.app

@UziTech
Copy link
Member

UziTech commented Jun 12, 2020

Can you write a test to make sure no one breaks it in the future.

@jun-sheaf
Copy link
Contributor Author

The tests have been included.

Copy link
Member

@UziTech UziTech left a 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.

src/Tokenizer.js Outdated Show resolved Hide resolved
src/rules.js Outdated Show resolved Hide resolved
@@ -345,19 +345,27 @@ a | b
md: `
1. item 1
2. item 2
3) item 3
4) item 4
Copy link
Member

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>

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So shouldFail means shouldNotFailButDoesFail?

Copy link
Member

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>
@UziTech UziTech changed the title Fixes #1700 Fixes ordered list ")" delimiter Jun 17, 2020
@UziTech UziTech requested a review from davisjam June 17, 2020 17:18
@UziTech UziTech requested review from joshbruce and styfle June 17, 2020 17:18
@UziTech
Copy link
Member

UziTech commented Jun 17, 2020

We require 2 Approvals for PRs so this will be merged when another team member approves it.

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Thanks!

@styfle styfle merged commit bd4757f into markedjs:master Jun 17, 2020
@UziTech UziTech mentioned this pull request Jul 13, 2020
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ordered list doesn't support ")" as delimiter
3 participants