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

Improve BlockCode function #17

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@bogem

bogem commented Nov 5, 2017

Actually, I didn't find any example of usage, when for _, elt := range strings.Fields(lang) { ... } loop is useful. That's why I deleted it. If I'm not right, please explain me, which situations this loop manages.

@dmitshur

Thanks for the PR. I'll review it more thoroughly in the coming days. For now, just answering questions/providing information.

@@ -135,42 +135,31 @@ func extractText(n *html.Node) string {
}
// TODO: Clean up and improve this code.
// GitHub Flavored Markdown fenced code block with highlighting.
// BlockCode renders GitHub Flavored Markdown fenced code block with highlighting.

This comment has been minimized.

@dmitshur

dmitshur Nov 7, 2017

Member

This is a good change, I'll happily take it.

@dmitshur

dmitshur Nov 7, 2017

Member

This is a good change, I'll happily take it.

break
lang = strings.TrimSpace(lang)
if len(lang) > 0 && lang[0] == '.' {
lang = lang[1:]

This comment has been minimized.

@dmitshur

dmitshur Nov 7, 2017

Member

Actually, I didn't find any example of usage, when for _, elt := range strings.Fields(lang) { ... } loop is useful. That's why I deleted it. If I'm not right, please explain me, which situations this loop manages.

The original code for BlockCode is largely taken from blackfriday.Html.BlockCode, with minor GFM-specific modifications applied. That includes the for _, elt := strings.Fields(lang) loop. You can see it upstream at:

https://github.com/russross/blackfriday/blob/6d1ef893fcb01b4f50cb6e57ed7df3e2e627b6b2/html.go#L258-L287

@dmitshur

dmitshur Nov 7, 2017

Member

Actually, I didn't find any example of usage, when for _, elt := range strings.Fields(lang) { ... } loop is useful. That's why I deleted it. If I'm not right, please explain me, which situations this loop manages.

The original code for BlockCode is largely taken from blackfriday.Html.BlockCode, with minor GFM-specific modifications applied. That includes the for _, elt := strings.Fields(lang) loop. You can see it upstream at:

https://github.com/russross/blackfriday/blob/6d1ef893fcb01b4f50cb6e57ed7df3e2e627b6b2/html.go#L258-L287

This comment has been minimized.

@bogem

bogem Nov 7, 2017

Okay, I will revert it. But anyway I can't understand, where it could be useful.

@bogem

bogem Nov 7, 2017

Okay, I will revert it. But anyway I can't understand, where it could be useful.

This comment has been minimized.

@dmitshur

dmitshur Nov 8, 2017

Member

But anyway I can't understand, where it could be useful.

In that case, I think a good next step is to open an issue upstream in blackfriday and discuss it there. If we can clarify or improve the implementation there, it'll be trivial to backport the improvements here.

@dmitshur

dmitshur Nov 8, 2017

Member

But anyway I can't understand, where it could be useful.

In that case, I think a good next step is to open an issue upstream in blackfriday and discuss it there. If we can clarify or improve the implementation there, it'll be trivial to backport the improvements here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment