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

Markdown: fix handling of code fences appearing on the same line #5606

Merged
merged 2 commits into from Apr 12, 2018

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Jan 18, 2018

In conformance with http://spec.commonmark.org/0.12/#fenced-code-blocks

To avoid detecting this whole line as the language of the code:

```invisible man```
Expected: "<p><code></code><code>invisible man</code><code></code></p>"
     got: "<pre><code class='language-invisible man```'></code></pre>"

Also delete an unused method

@@ -70,6 +70,8 @@ describe Markdown do
assert_render "```crystal\nHello\nWorld\n```", "<pre><code class='language-crystal'>Hello\nWorld</code></pre>"
assert_render "Hello\n```\nWorld\n```", "<p>Hello</p>\n\n<pre><code>World</code></pre>"
assert_render "```\n---\n```", "<pre><code>---</code></pre>"
assert_render "````\n---\n````", "<pre><code>---</code></pre>"
assert_render "```invisible man```", "<p><code></code><code>invisible man</code><code></code></p>"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is having 2 empty code block around the actual code correct?

Copy link
Member

Choose a reason for hiding this comment

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

Because there's no inline code fence, so it's an empty inline code block (``), an inline code block with the "invisible man" content and finally another empty inline code block (``)

Copy link
Member

Choose a reason for hiding this comment

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

Wow, thank you! I had the same doubt.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not so much about what's correct, just that the previous result was much, much worse (see first message). And typing like this is apparently quite common.

Copy link
Contributor

@bew bew Jan 18, 2018

Choose a reason for hiding this comment

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

According to this example of the spec: http://spec.commonmark.org/0.12/#example-95
There should just be one <code> block, but as you said, the old behavior was worst, maybe it's ok to do this in another pr.

Copy link
Member

Choose a reason for hiding this comment

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

Code spans can be delimited by any number of backticks, the only requirement is that start and end delimiter have equal length. So ```invisible man```should really only produce <p><code>invisble man</code></p>. See CommonMark spec: Code spans

Copy link
Member Author

Choose a reason for hiding this comment

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

Inline code blocks are not affected by this PR and are outside the scope.

e.g. this is the same before and after.

require "markdown"; p Markdown.to_html("a ```b```")'
"<p>a <code></code><code>b</code><code></code></p>"

I added a TODO for the spec.

In conformance with http://spec.commonmark.org/0.12/#fenced-code-blocks

To avoid detecting this whole line as the language of the code:
```invisible man```
Expected: "<p><code></code><code>invisible man</code><code></code></p>"
     got: "<pre><code class='language-invisible man```'></code></pre>"

Also delete an unused method
@oprypin
Copy link
Member Author

oprypin commented Apr 10, 2018

Ping

Copy link
Member

@jhass jhass left a comment

Choose a reason for hiding this comment

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

I would prefer to go full way and have the TODO removed here, but if another contributor things it's fine as is I'm fine with it too to solve the immediate issue.

@sdogruyol
Copy link
Member

This fixes the described issue. Let's do a separate PR for the full dive for more improvements 👍

@sdogruyol sdogruyol added this to the Next milestone Apr 12, 2018
@sdogruyol sdogruyol merged commit ba77124 into crystal-lang:master Apr 12, 2018
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.

None yet

6 participants