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

Don’t recognize U+2028 as a newline character #581

Merged
merged 2 commits into from Aug 11, 2019
Merged

Don’t recognize U+2028 as a newline character #581

merged 2 commits into from Aug 11, 2019

Conversation

@puzrin
Copy link
Member

puzrin commented Aug 11, 2019

Hi Mathias! As far as i see, commonmark spec mentions only 0x0A & 0x0D https://spec.commonmark.org/0.29/#preliminaries.

You are right, we need to clarify condition. What can you say about the rest of characters? \u0085, \u2424? What is better way to process those? Formally, we shold remove those too if we need to follow CM spec strictly. But what about real world?

@mathiasbynens
Copy link
Contributor Author

mathiasbynens commented Aug 11, 2019

Ooh, good point — it would be nice to match CommonMark exactly, which would be even simpler! Happy to tweak this patch accordingly if you decide so.

Could you share any background on why those other characters (U+0085, U+2424) were added to markdown-it in the first place? Was there user demand for them somehow?

@puzrin
Copy link
Member

puzrin commented Aug 11, 2019

To be honest, i don't remember. Probably, i imagined myself to be more clever than spec authors :). Need to search CM forum, if anyone did such requests.

What is your personal advice for me, according to your experience? Remove everything and leave 0x0A & 0x0D only?

@mathiasbynens
Copy link
Contributor Author

What is your personal advice for me, according to your experience? Remove everything and leave 0x0A & 0x0D only?

I’d personally prefer to only do what CM requires, indeed. My PR was initially a smaller change (just removing U+2028 from the set) because I assumed there would be opposition to the other removals.

I’ve just updated the patch accordingly. Please take a look — thanks!

@puzrin puzrin merged commit 39a35f4 into markdown-it:master Aug 11, 2019
@puzrin
Copy link
Member

puzrin commented Aug 11, 2019

Thank you, merged.

Should i release ASAP it that's not urgent?

@mathiasbynens mathiasbynens deleted the patch-1 branch August 11, 2019 12:26
@mathiasbynens
Copy link
Contributor Author

I’d love to see this in a release soon :) But don’t worry, if it’s too much trouble, I’ll just npm install the master branch for now.

@puzrin
Copy link
Member

puzrin commented Aug 11, 2019

Released.

@mathiasbynens
Copy link
Contributor Author

Thank you for the quick turnaround!

mathiasbynens added a commit to v8/v8.dev that referenced this pull request Aug 11, 2019
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.

None yet

2 participants