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

Making `highlight` behave more like redcarpet #2511

Merged
merged 2 commits into from Jun 23, 2014

Conversation

Projects
None yet
4 participants
@denilsonsa
Contributor

denilsonsa commented Jun 13, 2014

See issue #2510 for details.

Note: redcarpet does not escape the class name (like Jekyll does).

This means that c++ in redcarpet generates class="language-c++", while in Jekyll will be c--. Should I also remove this gsub("+", "-") call, so that both generate the same code? And why is it here anyway?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 13, 2014

Member

LGTM!

Member

parkr commented Jun 13, 2014

LGTM!

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr
Member

parkr commented Jun 13, 2014

parkr added a commit that referenced this pull request Jun 23, 2014

@parkr parkr merged commit e75dcc1 into jekyll:master Jun 23, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

parkr added a commit that referenced this pull request Jun 23, 2014

@denilsonsa

This comment has been minimized.

Show comment
Hide comment
@denilsonsa

denilsonsa Jun 23, 2014

Contributor

@parkr Nobody answered me about that gsub call in the code. But it is okay, it is not very important. (The issue fixed by this pull request was much worse, thanks for merging it!)

Contributor

denilsonsa commented Jun 23, 2014

@parkr Nobody answered me about that gsub call in the code. But it is okay, it is not very important. (The issue fixed by this pull request was much worse, thanks for merging it!)

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 23, 2014

Member

Sorry! It looked fine to me (c++ would be tranformed into c--). + is a special char in CSS?

Member

parkr commented Jun 23, 2014

Sorry! It looked fine to me (c++ would be tranformed into c--). + is a special char in CSS?

@denilsonsa

This comment has been minimized.

Show comment
Hide comment
@denilsonsa

denilsonsa Jun 23, 2014

Contributor

According to this StackOverflow question, essentially all non-whitespace chars are valid (they might just need escaping).

It just seemed odd to me to explicity change + to - in that code, with no comment to why it is needed. After all, why change + from c++ but not change # from c#? (BTW, both have alternative names: cpp and csharp.)

In my opinion, I believe it is best for the users if both Jekyll and redcarpet agree on the output. Either both of them escape/replace equally, or none of them do it. For simplicity, I'd stick to the later.

Your call. :)

(BTW, you may also want to close #2510.)

Contributor

denilsonsa commented Jun 23, 2014

According to this StackOverflow question, essentially all non-whitespace chars are valid (they might just need escaping).

It just seemed odd to me to explicity change + to - in that code, with no comment to why it is needed. After all, why change + from c++ but not change # from c#? (BTW, both have alternative names: cpp and csharp.)

In my opinion, I believe it is best for the users if both Jekyll and redcarpet agree on the output. Either both of them escape/replace equally, or none of them do it. For simplicity, I'd stick to the later.

Your call. :)

(BTW, you may also want to close #2510.)

@denilsonsa denilsonsa deleted the denilsonsa:patch-1 branch Jul 10, 2015

@leobalter leobalter referenced this pull request Sep 21, 2015

Closed

punctuation not visible #123

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Sep 21, 2015

Not that having changed class names from html to language-html represented a breaking change. I understand the examples with c and c++ are enough arguments for this change but it would be easier to identify this change in a major version, in case you're using a semver standard.

Nothing to fix after for this comment but I ask to avoid breaking retrocompatibility on minor versions next time to prevent a time investigating what happened.

leobalter commented Sep 21, 2015

Not that having changed class names from html to language-html represented a breaking change. I understand the examples with c and c++ are enough arguments for this change but it would be easier to identify this change in a major version, in case you're using a semver standard.

Nothing to fix after for this comment but I ask to avoid breaking retrocompatibility on minor versions next time to prevent a time investigating what happened.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 21, 2015

Member

@leobalter We do our best.

Member

parkr commented Sep 21, 2015

@leobalter We do our best.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Sep 22, 2015

Sure, and Jekyll is a great tool! I'm sorry if was tough.

leobalter commented Sep 22, 2015

Sure, and Jekyll is a great tool! I'm sorry if was tough.

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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