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

Fixed https://github.com/chjj/marked/issues/465 #974

Closed
wants to merge 2 commits into from
Closed

Fixed https://github.com/chjj/marked/issues/465 #974

wants to merge 2 commits into from

Conversation

KostyaTretyak
Copy link
Contributor

No description provided.

@joshbruce
Copy link
Member

@matt- or @UziTech - Can you check this out.

@KostyaTretyak
Copy link
Contributor Author

KostyaTretyak commented Dec 18, 2017

I split my commit into two commits. To make it clearer what I am fixing, see 98ac7a4 and a477d1d

@UziTech
Copy link
Member

UziTech commented Dec 18, 2017

LGTM

The bug that code was meant to fix (#77) seems to still work

@KostyaTretyak
Copy link
Contributor Author

@UziTech, when I do the following (with this my PR):

const marked = require('./');

const str = `[link][1]

* List

  [1]: http://example.com
`;

const md = marked(str);
console.log(md);

marked output this:

<p><a href="http://example.com">link</a></p>
<ul>
<li>List</li>
</ul>

What is exactly awaits @tlvince.

@KostyaTretyak
Copy link
Contributor Author

By the way, and without my PR, it works as expected.

@@ -39,10 +39,6 @@ block.list = replace(block.list)
('def', '\\n+(?=' + block.def.source + ')')
();

block.blockquote = replace(block.blockquote)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is plain wrong, cannot remove this without changing the block.blockquote regex

@@ -6,7 +6,8 @@
<hr>

<blockquote>
<p>hello</p>
<p>hello
[2]: hello</p>
Copy link
Contributor

@Feder1co5oave Feder1co5oave Jan 8, 2018

Choose a reason for hiding this comment

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

from def_blocks.text:

> hello
[2]: hello

I'm not sure this should/should not be parsed as a link definition. It is outside a blockquote.

Copy link
Contributor Author

@KostyaTretyak KostyaTretyak Jan 8, 2018

Choose a reason for hiding this comment

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

As we now know, this edit was correct because we have rule A link reference definition cannot interrupt a paragraph.

Copy link
Contributor

Choose a reason for hiding this comment

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

http://spec.commonmark.org/0.28/#example-210
Ok, because of the laziness rule, '[2]: hello' is part of the blockquote, too, and of the same paragraph of the previous line. So it is not a link def. This is correct then!

@@ -24,5 +25,6 @@
<blockquote>
<p>foo
bar
[1]: foo
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a problem in https://github.com/KostyaTretyak/marked/blob/98ac7a43958014832788b9d01580f394c84bfb27/test/tests/def_blocks.text :
The [1] at line 2 is interpreted as a shortcut link to link definition at line 20, so it can be confusing. However, I think it (line 2) should be parsed as a link def itself, because it follows the correct syntax, event if it is inside a blockquote, because it is allowed by the commonmark spec.

Copy link
Contributor Author

@KostyaTretyak KostyaTretyak Jan 8, 2018

Choose a reason for hiding this comment

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

As we now know, this edit was correct because we have rule A link reference definition cannot interrupt a paragraph.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no broad consensus on this one:
http://johnmacfarlane.net/babelmark2/?normalize=1&text=%3E+foo%0A%3E+bar%0A%5B1%5D%3A+foo%0A%3E+bar

However, commonmark.js parses it as you wrote.

@Feder1co5oave
Copy link
Contributor

I have worked on fixing rules for link defs, they should pass the def_blocks test as written by @KostyaTretyak (which we've deemed correct per commonmark), and also almost every example from commonmark. I will submit a pr once I've reviewed and rearranged commits.

Feder1co5oave added a commit to Feder1co5oave/marktex that referenced this pull request Jan 20, 2018
…agraph + blockquote laziness rule, according to commonmark spec

(see review of markedjs#974. Partial replay of 98ac7a4)
zhenalexfan pushed a commit to zhenalexfan/MarkdownHan that referenced this pull request Nov 8, 2021
…agraph + blockquote laziness rule, according to commonmark spec

(see review of markedjs#974. Partial replay of 98ac7a4)
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.

4 participants