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

Fix #3371 - kramdown:syntax_highlighter should take value of highlighter #4090

Merged
merged 2 commits into from Nov 2, 2015

Conversation

Projects
None yet
4 participants
@paulrobertlloyd
Contributor

paulrobertlloyd commented Nov 1, 2015

A fix for #3371

I’ve added a test that checks to see that code fenced syntax is rendered using Rouge. However, this test (and fix?) assumes you have Rouge as your preferred highlighter. I tried enabling Coderay highlighting, but this appeared to have no effect, either on syntax within {% highlight %} or ~~~ code fences. Is this a bug, or intended behaviour?

Also, as kramdown only accepts Rouge and Coderay as syntax highlighters, maybe we should only allow those options to be passed to the kramdown: syntax_highlighter config?

@paulrobertlloyd

This comment has been minimized.

Show comment
Hide comment
@paulrobertlloyd

paulrobertlloyd Nov 1, 2015

Contributor

Hmm, not sure why this is failing in Travis, but not when I run the tests locally… 🤔

Contributor

paulrobertlloyd commented Nov 1, 2015

Hmm, not sure why this is failing in Travis, but not when I run the tests locally… 🤔

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 1, 2015

Contributor

Hulk: ++prepare+patch,merge+and+trace report:ONLYME trace:true stack:true track:callers+with+source

Contributor

envygeeks commented Nov 1, 2015

Hulk: ++prepare+patch,merge+and+trace report:ONLYME trace:true stack:true track:callers+with+source

@envygeeks envygeeks self-assigned this Nov 1, 2015

@envygeeks envygeeks added this to the 3.0.1 milestone Nov 1, 2015

@envygeeks envygeeks added Bug labels Nov 1, 2015

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 1, 2015

Member

Hulk looks pretty cool.

@paulrobertlloyd Are you testing on Ruby 2.2? It succeeds for all the others, just fails on script/test in Ruby 2.2. I just kicked the build, let's see if it fails again.

Thanks!

Member

parkr commented Nov 1, 2015

Hulk looks pretty cool.

@paulrobertlloyd Are you testing on Ruby 2.2? It succeeds for all the others, just fails on script/test in Ruby 2.2. I just kicked the build, let's see if it fails again.

Thanks!

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 1, 2015

Member

I tried enabling Coderay highlighting, but this appeared to have no effect, either on syntax within {% highlight %} or ~~~ code fences. Is this a bug, or intended behaviour?

I'm not sure if Coderay is supported. I think @envygeeks knows more about this.

maybe we should only allow those options to be passed to the kramdown: syntax_highlighter config?

Yes, sanitization is good!

Member

parkr commented Nov 1, 2015

I tried enabling Coderay highlighting, but this appeared to have no effect, either on syntax within {% highlight %} or ~~~ code fences. Is this a bug, or intended behaviour?

I'm not sure if Coderay is supported. I think @envygeeks knows more about this.

maybe we should only allow those options to be passed to the kramdown: syntax_highlighter config?

Yes, sanitization is good!

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 1, 2015

Contributor

I'm not sure if Coderay is supported. I think @envygeeks knows more about this.

@parkr In Jekyll coderay isn't supported because we don't accept the syntax_highlighter option: https://github.com/paulrobertlloyd/jekyll/blob/9d1641f163e6b20507436872336414cf6df931c7/lib/jekyll/converters/markdown/kramdown_parser.rb#L18 I vote we enable it but don't let it block this pull, I have the tags pull coming today so I'll send up a second pull to enable syntax_highlighter if ya'll want unless you want to do it @paulrobertlloyd

Contributor

envygeeks commented Nov 1, 2015

I'm not sure if Coderay is supported. I think @envygeeks knows more about this.

@parkr In Jekyll coderay isn't supported because we don't accept the syntax_highlighter option: https://github.com/paulrobertlloyd/jekyll/blob/9d1641f163e6b20507436872336414cf6df931c7/lib/jekyll/converters/markdown/kramdown_parser.rb#L18 I vote we enable it but don't let it block this pull, I have the tags pull coming today so I'll send up a second pull to enable syntax_highlighter if ya'll want unless you want to do it @paulrobertlloyd

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 1, 2015

Contributor

Actually re-reading that code entirely after drinking some coffee points out that we need to readjust that method to explicitly enable all that on the latest Kramdown. So once this is merged I'll ship that too.

Contributor

envygeeks commented Nov 1, 2015

Actually re-reading that code entirely after drinking some coffee points out that we need to readjust that method to explicitly enable all that on the latest Kramdown. So once this is merged I'll ship that too.

@@ -5,6 +5,11 @@ class KramdownParser
def initialize(config)
require 'kramdown'
@config = config
# If kramdown supported highlighter enabled, use that
highlighter = @config['highlighter']
if highlighter == 'rouge' || highlighter == 'coderay'

This comment has been minimized.

@paulrobertlloyd

paulrobertlloyd Nov 1, 2015

Contributor

@parkr I’ve added this extra check so that we only set ['kramdown']['syntax_highlighter'] if rouge or coderay are set as the global highlighter. I’m a Ruby newbie, so please tell me if this works, or can be optimised! Thanks.

@paulrobertlloyd

paulrobertlloyd Nov 1, 2015

Contributor

@parkr I’ve added this extra check so that we only set ['kramdown']['syntax_highlighter'] if rouge or coderay are set as the global highlighter. I’m a Ruby newbie, so please tell me if this works, or can be optimised! Thanks.

@paulrobertlloyd

This comment has been minimized.

Show comment
Hide comment
@paulrobertlloyd

paulrobertlloyd Nov 1, 2015

Contributor

@envygeeks I’ll let you make the changes you need regarding correct coderay support. Much quicker/less error-prone that way I bet!

Contributor

paulrobertlloyd commented Nov 1, 2015

@envygeeks I’ll let you make the changes you need regarding correct coderay support. Much quicker/less error-prone that way I bet!

parkr added a commit that referenced this pull request Nov 2, 2015

@parkr parkr merged commit 05a982e into jekyll:master Nov 2, 2015

1 check passed

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

parkr added a commit that referenced this pull request Nov 2, 2015

@m5o m5o referenced this pull request Feb 28, 2016

Closed

use Rouge as syntax highlighter #17

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