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

Use valid css class names when pygments language contains invalid chars, eg html+django #994

Merged
merged 2 commits into from Mar 17, 2014

Conversation

Projects
None yet
4 participants
@stephenmcd
Contributor

stephenmcd commented Apr 22, 2013

Pygments has some lexers that combine multiple languages and are referenced by name using the plus sign, eg "html+django" is a valid lexer name.

In the highlight tag, the lexer name also gets used as the CSS class name in the wrapping HTML <code> block, but the plus sign is invalid here.

This change simply converts plus signs to minus signs, to conform with valid CSS class names.

@parkr

This comment has been minimized.

Member

parkr commented Apr 22, 2013

I think we'd want to normalize the lang elsewhere, right? And I'd love tests for this so we don't break it in the future. They're in ./test/test_tags.rb.

@stephenmcd

This comment has been minimized.

Contributor

stephenmcd commented Apr 22, 2013

Is it used as a CSS classname anywhere else? As far as Pygments goes it's doing the right thing, so nothing needs to change there.

As for tests I haven't written Ruby for a couple of years, so I'm a bit out of my depth there. Happy to give it a go but I'd need some guidance.

@mattr-

This comment has been minimized.

Member

mattr- commented Apr 23, 2013

@stephenmcd I'd be happy to pair with you remotely on this

@stephenmcd

This comment has been minimized.

Contributor

stephenmcd commented Apr 23, 2013

Thanks @mattr- !

Might not be right away - I've a ton of outstanding work on my own projects unfortunately. But if I get around to it I'll ping you here.

@parkr parkr closed this Mar 17, 2014

@stephenmcd

This comment has been minimized.

Contributor

stephenmcd commented Mar 17, 2014

Did this get fixed?

@parkr parkr reopened this Mar 17, 2014

@parkr

This comment has been minimized.

Member

parkr commented Mar 17, 2014

Nope, it was just closed by accident.

@stephenmcd

This comment has been minimized.

Contributor

stephenmcd commented Mar 17, 2014

Cool!

Would you like to go ahead and merge the fix? I don't do Ruby or anything with Jekyll these days, so I have almost no exposure to the code base - but as I understood, the line changed in this fix is the only location that deals with mapping language names to CSS class names, which the issue specifically is.

Doesn't seem worthwhile to leave a bug opened for this long when the fix is literally as trivial as they get. Just my 2 cents as someone who does way too much open source maintenance :-)

@parkr

This comment has been minimized.

Member

parkr commented Mar 17, 2014

So this fix is pretty trivial but it doesn't take into account @lang being nil, in which case it'll just freak out. Can you call .to_s first, before gsub? Thanks!

@stephenmcd

This comment has been minimized.

Contributor

stephenmcd commented Mar 17, 2014

No sweat, all done - thanks a lot!

parkr added a commit that referenced this pull request Mar 17, 2014

@parkr parkr merged commit d80471c into jekyll:master Mar 17, 2014

1 check was pending

default The Travis CI build is in progress
Details
@parkr

This comment has been minimized.

Member

parkr commented Mar 17, 2014

Boom! Thanks @stephenmcd :)

parkr added a commit that referenced this pull request Mar 17, 2014

lmullen added a commit to lmullen/jekyll that referenced this pull request Mar 24, 2014

@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.