Skip to content
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

Add support for the Rouge syntax highlighter #1859

Merged
merged 9 commits into from Dec 31, 2013

Conversation

Projects
None yet
8 participants
@robin850
Copy link
Contributor

robin850 commented Dec 22, 2013

Hello,

This is another shot at #930 since the Jekyll repository has moved since the opening of the latter. So this pull request adds support for the Rouge syntax highlighter which is a nice alternative to Pygments.

Here is the work done on this patch:

  • Improve the testing of the highlight tag when Pygments is enabled.
  • Rename the pygments configuration option to highlighter to easily add new highlighters in the future
  • Add support for Rouge
  • Update the documentation to match these changes

Let me know if I should change anything else.

Have a nice day.

robin850 added some commits Dec 22, 2013

Fix the highlight tag feature
Previously, the assertion made wasn't enough to check whether the code
block was correctly parsed through Pygments (and it was not the case).
This commit simply ensure there is a div with the "highlight" class and
fix the test to correctly invoke the Liquid tag rendering.
Rename the pygments option to highlighter
Rename the pygments configuration option to highlighter to allow
different highlighters in the future. For now, the allowed values are
`pygments` and `null`.

It's now more straightforward to plug another syntax highlighter.
@jneen

This comment has been minimized.

Copy link

jneen commented on lib/jekyll/configuration.rb in 9206413 Dec 22, 2013

You've got a typo on the end of the line here. I think you want an end quote, a comma, and a space. ;)

Also, it might be courteous to interpret pygments: true the same as highlighter: pygments until the deprecation notice is removed?

@jneen

This comment has been minimized.

Copy link

jneen commented on 9206413 Dec 22, 2013

🤘

@robin850

This comment has been minimized.

Copy link
Contributor Author

robin850 commented Dec 22, 2013

@jayferd : I've added some more commits ; thanks for the quick feedback! ❤️

@@ -362,7 +362,7 @@ Jekyll handles two special Redcarpet extensions:
# ...ruby code
```
With both fenced code blocks and pygments enabled, this will statically highlight the code; without pygments, it will add a `class="LANGUAGE"` attribute to the `<code>` element, which can be used as a hint by various JavaScript code highlighting libraries.
With both fenced code blocks an highlighter enabled, this will statically highlight the code; without any syntax highlighter, it will add a `class="LANGUAGE"` attribute to the `<code>` element, which can be used as a hint by various JavaScript code highlighting libraries.

This comment has been minimized.

@maul-esel

maul-esel Dec 22, 2013

Contributor

Lost a "d" here.

This comment has been minimized.

@robin850

robin850 Dec 22, 2013

Author Contributor

Good catch thank you!

robin850 added some commits Dec 22, 2013

Add support for the Rouge syntax highlighter
By setting the `highlighter` setting to `rouge` you can now easily
highlight your code with it instead of relying on Pygments. However,
Jekyll doesn't depend on Rouge explicitly, you will need to install it
or add it to your Gemfile.

The documentation has been updated accordingly.
Set highlighter to pygments when upgrading
In case you are upgrading from 1.4.2 to 2.0 and the pygments option is
set to true, then the highlighter option will be set to pygments
automatically.
Rely on the Redcarpet plugin instead of hard-coding
To avoid code duplication and have to keep tracking of the API change of
Rouge, let's rely on the Redcarpet plugin and customize the output on
our needs.
@parkr

This comment has been minimized.

Copy link
Member

parkr commented Dec 22, 2013

Whoa, this is a lot. I'll go through it and try to give my thoughts. In the future, please submit just one PR per eventual line in History.markdown and please let us update History.markdown (it's part of the PR merge process for us).

Given I have an "index.html" file that contains "{% highlight ruby %} puts 'Hello world!' {% endhighlight %}"
And I have a configuration file with "pygments" set to "true"
Given I have an "index.html" page that contains "{% highlight ruby %} puts 'Hello world!' {% endhighlight %}"
And I have a configuration file with "highlighter" set to "pygments"

This comment has been minimized.

@parkr

parkr Dec 22, 2013

Member

We should make sure it works without this line, depending upon the default. If it's not set, to which should it default?

This comment has been minimized.

@robin850

robin850 Dec 23, 2013

Author Contributor

Actually you are right ; this line is not needed since this parameter should default to pygments. I think we can simply remove it.

Jekyll.logger.warn "Deprecation:", "The 'pygments' configuration option" +
" has been renamed to 'highlighter'. Please update your" +
" config file accordingly. The allowed values are 'rouge', " +
"'pygments' or null"

This comment has been minimized.

@parkr

parkr Dec 22, 2013

Member

Would you mind adding a period to the end of this sentence? :) Great catch for the deprecation message, though. :)

@@ -5,7 +5,7 @@ class RedcarpetParser

module CommonMethods
def add_code_tags(code, lang)
code = code.sub(/<pre>/, "<pre><code class=\"#{lang} language-#{lang}\" data-lang=\"#{lang}\">")
code = code.sub(/<pre(.*?)>/, "<pre><code class=\"#{lang} language-#{lang}\" data-lang=\"#{lang}\">")

This comment has been minimized.

@parkr

parkr Dec 22, 2013

Member

Why is this needed?

This comment has been minimized.

@robin850

robin850 Dec 23, 2013

Author Contributor

This is actually needed because Rouge will yield a pre tag with a class attribute anyway (even if we specify an empty string as the CSS class ; see here). As you can see, the only way to avoid this is to set the wrap option to false when initializing the new Formatter but if do so, we can't rely on the Redcarpet plugin which makes the code a lot clearer I think.

Let me know if you prefer not to rely on this plugin and avoid this regex change ; I will update accordingly.

This comment has been minimized.

@jneen

jneen Dec 23, 2013

Actually, you can pass :wrap => false to the Formatter constructor to get the formatted html without the <pre> wrapper (see the if @wrap at the end of that line you linked). Then you can wrap it however you like :).

This comment has been minimized.

@robin850

robin850 Dec 23, 2013

Author Contributor

@jayferd : Yup but then we can't rely on the Redcarpet plugin ; I've updated the highlight tag with this change to avoid adding the (.*?) to the regex in this case but here, we can't do so much. 😃

This comment has been minimized.

@jneen

jneen Dec 23, 2013

Oh right. Derp. Well... what if I added a #rouge_formatter method that you could override?

This comment has been minimized.

@parkr

parkr Dec 25, 2013

Member

I'm never a big fan of >= in gemspecs, but ~> 1.3 would be totally fine. That requires between 1.3.0 and 2.0.0 :)

This comment has been minimized.

@robin850

robin850 Dec 25, 2013

Author Contributor

@parkr : But how could we handle this in the gemspec ? Actually the user should not have to depend on Rouge directly.

This comment has been minimized.

@parkr

parkr Dec 25, 2013

Member

@robin850 If we make Rouge optional, then we could add a check for the version, like

require 'rouge'
if Rouge::VERSION < '1.3.0'
  abort "Please install Rouge 1.3.0 or greater and try running Jekyll again."
end

Then we can catch version issues without requiring that rouge be installed.

This comment has been minimized.

@robin850

robin850 Dec 25, 2013

Author Contributor

Thanks for your comments guys, this should be updated in 036cbda!

This comment has been minimized.

@parkr

parkr Dec 25, 2013

Member

❤️

@@ -9,8 +9,7 @@ def self.process(args)
arg_is_present? args, "--auto", "The switch '--auto' has been replaced with '--watch'."
arg_is_present? args, "--no-auto", "To disable auto-replication, simply leave off \
the '--watch' switch."
arg_is_present? args, "--pygments", "The 'pygments' setting can only be set in \
your config files."
arg_is_present? args, "--pygments", "The 'pygments' setting has been removed"

This comment has been minimized.

@parkr

parkr Dec 22, 2013

Member

... has been removed in favour of 'highlighter'., maybe?

that</a> before you go any further.
want to enable syntax highlighting using Pygments or Rouge. You should
really <a href="../templates/#code_snippet_highlighting">check out how to
do that</a> before you go any further.

This comment has been minimized.

@parkr

parkr Dec 22, 2013

Member

Also further should probably be farther here. :)

@@ -139,8 +139,8 @@ your `excerpt_separator` to `""`.
## Highlighting code snippets

Jekyll also has built-in support for syntax highlighting of code snippets using
Pygments, and including a code snippet in any post is easy. Just use the
dedicated Liquid tag as follows:
either Pygments or [Rouge](https://github.com/jayferd/rouge), and including a

This comment has been minimized.

@parkr

parkr Dec 22, 2013

Member

Do you think we should link to both here instead of just to Rouge?

This comment has been minimized.

@robin850

robin850 Dec 23, 2013

Author Contributor

I agree with you ; we should either link to both or none of them. I've put a link for both in the installation.md page and remove this one.

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Dec 22, 2013

This LGTM besides my comments.

robin850 added some commits Dec 22, 2013

Set the wrap option to false when using Rouge
Since Rouge yields the pre tag with a class attribute but we don't want
it, we should set the wrap parameter to false when instantiating a new
formatter object.

Also use Rouge::Formatter#format instead of #render which is deprecated
and will be removed in the near future.
Remove a useless given step
Since the highlighter configuration option should default to pygments,
we don't have to explicitly set it in the step testing the output with
pygments.
@robin850

This comment has been minimized.

Copy link
Contributor Author

robin850 commented Dec 23, 2013

Whoa, this is a lot. I'll go through it and try to give my thoughts. In the future, please submit just one PR per eventual line in History.markdown and please let us update History.markdown (it's part of the PR merge process for us).

Really sorry ; I will next time! 😢

I've updated my branch with your comments. Thank you!

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Dec 24, 2013

Really sorry ; I will next time! 😢

No problem, just an FYI for the future. It makes it way easier for @mattr- and I to review but this isn't anything we can't handle. Your contribution is incredibly valuable to us so I appreciate it either way 😃

I've updated my branch with your comments. Thank you!

Brilliant! It all looks good to me. Will just want @mattr-'s 👀 on this and we'll be 👍 to 🚢.

Require at least 1.3.0 for Rouge
Rouge 1.3.0 introduced a `rouge_formatter` helper which is handy to
overwrite the formatter default when using the Redcarpet plugin so let's
require this version at the very least.

An abort statement will be thrown when the installed version is not
correct.

@ghost ghost assigned mattr- Dec 26, 2013

@siong1987

This comment has been minimized.

Copy link

siong1987 commented Dec 26, 2013

👍

@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented Dec 31, 2013

I merged another PR and this won't merge cleanly now. Could you rebase it against current master? Thanks!

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Dec 31, 2013

Conflict is only in History.markdown, should be a simple fix, @robin850. Thanks!

@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented Dec 31, 2013

oh, well, in that case, uno momento.

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Dec 31, 2013

You ok @mattr-? It's been 12 minutes 😜

@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented Dec 31, 2013

hey! these manual merges take time! i don't have automated script to do these yet 😝

mattr- added a commit that referenced this pull request Dec 31, 2013

@mattr- mattr- merged commit 036cbda into jekyll:master Dec 31, 2013

1 check passed

default The Travis CI build passed
Details
@parkr

This comment has been minimized.

Copy link
Member

parkr commented Dec 31, 2013

😉 Thanks for taking care of it ❤️

@jneen

This comment has been minimized.

Copy link

jneen commented Dec 31, 2013

:D

@robin850

This comment has been minimized.

Copy link
Contributor Author

robin850 commented Dec 31, 2013

Thank you guys and happy new year! ❤️ 💙 💛 💜 💚

@erikhuizinga

This comment has been minimized.

Copy link

erikhuizinga commented on site/docs/installation.md in 3ca2cb0 Sep 15, 2016

I think this should be ‘further’ instead of ‘farther’.

@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.
You can’t perform that action at this time.