Skip to content

Fix code fence handling. - #532

Open
teeler wants to merge 1 commit into
russross:v2from
teeler:fix-code-fences
Open

Fix code fence handling.#532
teeler wants to merge 1 commit into
russross:v2from
teeler:fix-code-fences

Conversation

@teeler

@teeler teeler commented Mar 19, 2019

Copy link
Copy Markdown

Parse info line after code fences even if the user doesn't ask for it, and keep it only if they want it.

Fixes #526

…e user doesn't ask for it, and keep it only if they want it.
@rtfb

rtfb commented Mar 24, 2019

Copy link
Copy Markdown
Collaborator

Hi @teeler, thanks for your contribution! A couple questions:

How does this fix #526? I see that the changes in block.go essentially address a TODO comment, which is a good thing in itself. However, I can't see how should that fix the problem described in the issue. Is there some subtle change in the logic hidden by this cleanup?

I am also a little bit concerned about those couple changes to the test cases since it's not clear what would be the desired behavior in this edge case. But since this is such an esoteric case, it may not be that important.

@teeler

teeler commented Mar 25, 2019

Copy link
Copy Markdown
Author

Hi,

I'm basically deleting the if guard and un-indenting the whole block here: https://github.com/russross/blackfriday/blob/v2.0.1/block.go#L610-L656

This block conditionally advances i based on info after the code fence, so the parser is left in an inconsistent state depending on if the caller wants code fence info or not (the code immediately following this block returns different things conditional on what is found at the ith index).

In the case where info is not requested but the code fence contains it, for example, the incorrect end of line is returned, which leaves the parser at a position making the rest of the input invalid/wrong.

As for the tests...the ~~~ lisp one looked wrong to me anyway? I'm not exactly sure what the behavior /should/ be for that but i tried it in a handful of markdown editors and none of them produced what is currently expected.

@evankanderson

Copy link
Copy Markdown

Ran into another case of #526 here, I think:

https://knative.dev/development/install/knative-with-aks/

@nicolasgarnier

Copy link
Copy Markdown

Could we merge this please?

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