Remove most runtime deps & move to Rouge as default highlighter #3323

Merged
merged 3 commits into from Jan 31, 2015

Conversation

Projects
None yet
10 participants
@parkr
Member

parkr commented Jan 18, 2015

This moves the following gems to as-wanted:

  • pygments.rb
  • redcarpet
  • toml
  • jekyll-paginate
  • jekyll-gist
  • jekyll-coffeescript
  • classifier-reborn

I'm on the fence about also taking jekyll-watch away... We're going to keep it.

/cc @jekyll/core

@parkr parkr changed the title from Remove most runtime deps to Remove most runtime deps & move to Rouge as default highlighter Jan 18, 2015

@parkr parkr added this to the 3.0 milestone Jan 18, 2015

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 18, 2015

Contributor

I'm very much 👍 on this.

Contributor

envygeeks commented Jan 18, 2015

I'm very much 👍 on this.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 18, 2015

Member

Once this is merged, I'd like to get a beta out into the wild.

Member

parkr commented Jan 18, 2015

Once this is merged, I'd like to get a beta out into the wild.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 18, 2015

Contributor

@parkr can the beta wait like 2 days past tomorrow? I can definitely finish cleaning up our tests into a cleaner Minitest state by then if it can wait, and we can get that out the door with it.

Contributor

envygeeks commented Jan 18, 2015

@parkr can the beta wait like 2 days past tomorrow? I can definitely finish cleaning up our tests into a cleaner Minitest state by then if it can wait, and we can get that out the door with it.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 18, 2015

Member

@parkr can the beta wait like 2 days past tomorrow? I can definitely finish cleaning up our tests into a cleaner Minitest state by then if it can wait, and we can get that out the door with it.

Beta on Tuesday? Works for me.

Member

parkr commented Jan 18, 2015

@parkr can the beta wait like 2 days past tomorrow? I can definitely finish cleaning up our tests into a cleaner Minitest state by then if it can wait, and we can get that out the door with it.

Beta on Tuesday? Works for me.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 18, 2015

Member

@envygeeks I'm also trying to figure out what tests are really slow because, yeah, some of them are terrible but I don't have a clear idea of which ones they are.

Member

parkr commented Jan 18, 2015

@envygeeks I'm also trying to figure out what tests are really slow because, yeah, some of them are terrible but I don't have a clear idea of which ones they are.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 18, 2015

Contributor

@parkr I actually added that in (because Minitest has an option for that) it seems most of the problem isn't the actual testing, it's the generation of the sites over and over again, if you want though I can always set the threshold smaller so we can track anything really expensive: d259637#diff-fa17210be88d091a513b7c0ed2cf5dafR27

Contributor

envygeeks commented Jan 18, 2015

@parkr I actually added that in (because Minitest has an option for that) it seems most of the problem isn't the actual testing, it's the generation of the sites over and over again, if you want though I can always set the threshold smaller so we can track anything really expensive: d259637#diff-fa17210be88d091a513b7c0ed2cf5dafR27

@parkr parkr referenced this pull request Jan 18, 2015

Closed

3.0 RELEASE GAMEPLAN #3324

7 of 7 tasks complete
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 18, 2015

Member

it seems most of the problem isn't the actual testing, it's the generation of the sites over and over again

Ah! Ok cool. Some of it might also be that we're still testing plugins. :x

Member

parkr commented Jan 18, 2015

it seems most of the problem isn't the actual testing, it's the generation of the sites over and over again

Ah! Ok cool. Some of it might also be that we're still testing plugins. :x

@paulrobertlloyd

This comment has been minimized.

Show comment
Hide comment
@paulrobertlloyd

paulrobertlloyd Jan 18, 2015

Contributor

I’d love to try this when it’s ready! Hopefully it will replace the current hacks I have in place to ensure kramdown and pygments play nice together.

Contributor

paulrobertlloyd commented Jan 18, 2015

I’d love to try this when it’s ready! Hopefully it will replace the current hacks I have in place to ensure kramdown and pygments play nice together.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 18, 2015

Member

kramdown and pygments

This simply changes the default highlighter to rouge, it doesn't alter the integration between kramdown and pygments, so I'm not sure what you mean.

Member

parkr commented Jan 18, 2015

kramdown and pygments

This simply changes the default highlighter to rouge, it doesn't alter the integration between kramdown and pygments, so I'm not sure what you mean.

@paulrobertlloyd

This comment has been minimized.

Show comment
Hide comment
@paulrobertlloyd

paulrobertlloyd Jan 18, 2015

Contributor

I mean, hopefully these new defaults will work better than the current defaults, which don't highlight code within fenced code blocks as expected (only code within {% highlight %} liquid blocks is parsed by pygments). See:

This has been a long frustration of mine! Happy to test this to see if Rouge highlights fenced code blocks as expected.

Contributor

paulrobertlloyd commented Jan 18, 2015

I mean, hopefully these new defaults will work better than the current defaults, which don't highlight code within fenced code blocks as expected (only code within {% highlight %} liquid blocks is parsed by pygments). See:

This has been a long frustration of mine! Happy to test this to see if Rouge highlights fenced code blocks as expected.

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Jan 18, 2015

Member

👍

I'd like to keep jekyll-watch in for now, since it's quite a core feature for the Jekyll CLI.

Member

alfredxing commented Jan 18, 2015

👍

I'd like to keep jekyll-watch in for now, since it's quite a core feature for the Jekyll CLI.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 18, 2015

Member

I'm on the fence about also taking jekyll-watch away...

It’s so easy to add A Gem to a Gemfile as needed, what’s the big advantage to keeping it bolted to core?

Member

pathawks commented Jan 18, 2015

I'm on the fence about also taking jekyll-watch away...

It’s so easy to add A Gem to a Gemfile as needed, what’s the big advantage to keeping it bolted to core?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 18, 2015

Contributor

It's already in there so you are posing the wrong question: What is the big advantage of removing it?

Contributor

envygeeks commented Jan 18, 2015

It's already in there so you are posing the wrong question: What is the big advantage of removing it?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 18, 2015

Member

The advantage of removing it is there are fewer gems to download. The disadvantage is that it makes it more obtuse. With jekyll serve defaulting to using watch, that would also cause problems.

Member

parkr commented Jan 18, 2015

The advantage of removing it is there are fewer gems to download. The disadvantage is that it makes it more obtuse. With jekyll serve defaulting to using watch, that would also cause problems.

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Jan 18, 2015

Contributor

For what is worth I too believe watch should be part of core being that it's the default with serve.

Big 👍 about Rouge!

Contributor

XhmikosR commented Jan 18, 2015

For what is worth I too believe watch should be part of core being that it's the default with serve.

Big 👍 about Rouge!

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 18, 2015

Member

For what is worth I too believe watch should be part of core being that it's the default with serve.

I guess since I never use serve, I wouldn't miss it.

Would decoupling it result in a lighter code review burden for GitHub Pages?

Member

pathawks commented Jan 18, 2015

For what is worth I too believe watch should be part of core being that it's the default with serve.

I guess since I never use serve, I wouldn't miss it.

Would decoupling it result in a lighter code review burden for GitHub Pages?

@kleinfreund

This comment has been minimized.

Show comment
Hide comment
@kleinfreund

kleinfreund Jan 18, 2015

Contributor

I guess since I never use serve, I wouldn't miss it.

Huh. I use jekyll s all the time.

Contributor

kleinfreund commented Jan 18, 2015

I guess since I never use serve, I wouldn't miss it.

Huh. I use jekyll s all the time.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 19, 2015

Contributor

I have to agree with @kleinfreund I rely on jekyll s to develop, it would drive me crazy to lose it out of the box, and I don't think it's loaded when Jekyll is not using it so I don't believe it impacts performance of production building at all.

Contributor

envygeeks commented Jan 19, 2015

I have to agree with @kleinfreund I rely on jekyll s to develop, it would drive me crazy to lose it out of the box, and I don't think it's loaded when Jekyll is not using it so I don't believe it impacts performance of production building at all.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 19, 2015

Member

Cool, thanks for the input everyone. I have crossed that line out. We'll keep it. It doesn't impact the security pressure on GitHub for Pages support, because, as @envygeeks said, it's not required unless explicitly used, and they don't use it.

Member

parkr commented Jan 19, 2015

Cool, thanks for the input everyone. I have crossed that line out. We'll keep it. It doesn't impact the security pressure on GitHub for Pages support, because, as @envygeeks said, it's not required unless explicitly used, and they don't use it.

@patrickwelker

This comment has been minimized.

Show comment
Hide comment
@patrickwelker

patrickwelker Jan 26, 2015

@paulrobertlloyd Sadly Jekyll 3.0 with kramdown and rouge doesn't support styling fenced code blocks. As before setting the liquid tag {% highlight %} is the only option as you can see in the demo code below:

<p>fenced code block with backticks</p>
<pre>
    <code class="language-ruby">def foo
      puts 'foo'
    end
    </code>
</pre>

<p>fenced codeblock with backticks</p>
<pre>
    <code class="language-ruby">def foo
      puts 'foo'
    end
    </code>
</pre>

<p>liquid tags</p>
<div class="highlight">
    <pre>
        <code class="language-ruby" data-lang="ruby">
            <span class="k">def</span>
            <span class="nf">foo</span>
            <span class="nb">puts</span>
            <span class="s1">'foo'</span>
            <span class="k">end</span>
        </code>
    </pre>
</div>

I'd also like to see support for fenced code blocks. It would be one problem less on the list and shipping Jekyll with support for such a popular format to write code blocks out of the box would be amazing.

@paulrobertlloyd Sadly Jekyll 3.0 with kramdown and rouge doesn't support styling fenced code blocks. As before setting the liquid tag {% highlight %} is the only option as you can see in the demo code below:

<p>fenced code block with backticks</p>
<pre>
    <code class="language-ruby">def foo
      puts 'foo'
    end
    </code>
</pre>

<p>fenced codeblock with backticks</p>
<pre>
    <code class="language-ruby">def foo
      puts 'foo'
    end
    </code>
</pre>

<p>liquid tags</p>
<div class="highlight">
    <pre>
        <code class="language-ruby" data-lang="ruby">
            <span class="k">def</span>
            <span class="nf">foo</span>
            <span class="nb">puts</span>
            <span class="s1">'foo'</span>
            <span class="k">end</span>
        </code>
    </pre>
</div>

I'd also like to see support for fenced code blocks. It would be one problem less on the list and shipping Jekyll with support for such a popular format to write code blocks out of the box would be amazing.

@paulrobertlloyd

This comment has been minimized.

Show comment
Hide comment
@paulrobertlloyd

paulrobertlloyd Jan 26, 2015

Contributor

@pattulus I’ve actually meaning to file bug against this… you can get kramdown to highlight fenced code blocks with Rouge, but you have to set it as a separate config option, as follows:

highlighter: rouge
kramdown:
  syntax_highlighter: rouge

(Strangely, this doesn't work if you wish to use Pygments).

Not sure if this is the expected behaviour, or even desirable (having to explicitly config a highlighter twice). Essentially highlighter relates to {% highlight ruby %}, while kramdown: syntax_highlighter: relates to ~~~ruby.

Contributor

paulrobertlloyd commented Jan 26, 2015

@pattulus I’ve actually meaning to file bug against this… you can get kramdown to highlight fenced code blocks with Rouge, but you have to set it as a separate config option, as follows:

highlighter: rouge
kramdown:
  syntax_highlighter: rouge

(Strangely, this doesn't work if you wish to use Pygments).

Not sure if this is the expected behaviour, or even desirable (having to explicitly config a highlighter twice). Essentially highlighter relates to {% highlight ruby %}, while kramdown: syntax_highlighter: relates to ~~~ruby.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 26, 2015

Member

@paulrobertlloyd Think we should enable the kramdown.syntax_highlighter automatically based on highlighter's value?

Member

parkr commented Jan 26, 2015

@paulrobertlloyd Think we should enable the kramdown.syntax_highlighter automatically based on highlighter's value?

@paulrobertlloyd

This comment has been minimized.

Show comment
Hide comment
@paulrobertlloyd

paulrobertlloyd Jan 26, 2015

Contributor

@parkr I think so… I mean, that’s the behaviour I expected, but I may be an outlier! I think that would be a sensible default (assuming you could override with kramdown.syntax_highlighter… or does that get even more complicated?)

(Short answer: yes!)

Contributor

paulrobertlloyd commented Jan 26, 2015

@parkr I think so… I mean, that’s the behaviour I expected, but I may be an outlier! I think that would be a sensible default (assuming you could override with kramdown.syntax_highlighter… or does that get even more complicated?)

(Short answer: yes!)

@patrickwelker

This comment has been minimized.

Show comment
Hide comment
@patrickwelker

patrickwelker Jan 26, 2015

@paulrobertlloyd That's a super-nifty trick. I hadn't set highlighter: rouge before. Now I have both of them set, like in your example. Still, using ~~~ruby won't show any highlighting, nor output any of the spans in the code tag.

Any ideas if there's something missing in my config file? I'd really like to use fenced code blocks.

source:        _src
encoding:      utf-8
safe:          false
future:        false
unpublished:   false
lsi:           false
baseurl: "" # the subpath of your site, e.g. /blog/
url:           http://derubercast.link
permalink:     /podcast/:title.html

markdown: kramdown
highlighter: rouge
kramdown:
    syntax_highlighter: rouge
    input: GFM
    auto_ids: true
#   footnote_nr: 1
#   entity_output: as_char
    toc_levels: 1..3
    smart_quotes: lsquo,rsquo,ldquo,rdquo

gems: ['bourbon']

@paulrobertlloyd That's a super-nifty trick. I hadn't set highlighter: rouge before. Now I have both of them set, like in your example. Still, using ~~~ruby won't show any highlighting, nor output any of the spans in the code tag.

Any ideas if there's something missing in my config file? I'd really like to use fenced code blocks.

source:        _src
encoding:      utf-8
safe:          false
future:        false
unpublished:   false
lsi:           false
baseurl: "" # the subpath of your site, e.g. /blog/
url:           http://derubercast.link
permalink:     /podcast/:title.html

markdown: kramdown
highlighter: rouge
kramdown:
    syntax_highlighter: rouge
    input: GFM
    auto_ids: true
#   footnote_nr: 1
#   entity_output: as_char
    toc_levels: 1..3
    smart_quotes: lsquo,rsquo,ldquo,rdquo

gems: ['bourbon']
@paulrobertlloyd

This comment has been minimized.

Show comment
Hide comment
@paulrobertlloyd

paulrobertlloyd Jan 27, 2015

Contributor

@pattulus Ah, if you’re using input: GFM you should use three backpacks, not three tildes. Hopefully that should then work.

Contributor

paulrobertlloyd commented Jan 27, 2015

@pattulus Ah, if you’re using input: GFM you should use three backpacks, not three tildes. Hopefully that should then work.

@patrickwelker

This comment has been minimized.

Show comment
Hide comment
@patrickwelker

patrickwelker Jan 27, 2015

@paulrobertlloyd Mh. Already tried it and replace all tildes with backticks. I also tested both versions without the GFM option. Strange that it doesn't work here.

@paulrobertlloyd Mh. Already tried it and replace all tildes with backticks. I also tested both versions without the GFM option. Strange that it doesn't work here.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 27, 2015

Contributor

Can we please move all this noise to a ticket of it's own?

Contributor

envygeeks commented Jan 27, 2015

Can we please move all this noise to a ticket of it's own?

@patrickwelker

This comment has been minimized.

Show comment
Hide comment
@patrickwelker

patrickwelker Jan 27, 2015

Sorry. I didn't want to hijack this thing.
Last note: I figured it out, I had to use bundle to install rouge rather than doing gem install (source)

Sorry. I didn't want to hijack this thing.
Last note: I figured it out, I had to use bundle to install rouge rather than doing gem install (source)

@paulrobertlloyd

This comment has been minimized.

Show comment
Hide comment
@paulrobertlloyd

paulrobertlloyd Jan 27, 2015

Contributor

Moved discussion of syntax highlighting config woes to a new issue: #3371 /cc @parkr @pattulus

Contributor

paulrobertlloyd commented Jan 27, 2015

Moved discussion of syntax highlighting config woes to a new issue: #3371 /cc @parkr @pattulus

@parkr parkr merged commit 3d843d8 into master Jan 31, 2015

1 check passed

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

@parkr parkr deleted the remove-runtime-deps branch Jan 31, 2015

parkr added a commit that referenced this pull request Jan 31, 2015

@mrzool

This comment has been minimized.

Show comment
Hide comment
@mrzool

mrzool Nov 20, 2015

I guess then the documentation isn't up to date on this?

screen shot 2015-11-20 at 18 01 22

mrzool commented Nov 20, 2015

I guess then the documentation isn't up to date on this?

screen shot 2015-11-20 at 18 01 22

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 20, 2015

Contributor

@mrzool yes, we no longer bundle pygments.rb, now we use rouge by default, the surface of it applies but it could use a slight reworking to imply that pygments is not included but optional and rouge is used instead now.

Contributor

envygeeks commented Nov 20, 2015

@mrzool yes, we no longer bundle pygments.rb, now we use rouge by default, the surface of it applies but it could use a slight reworking to imply that pygments is not included but optional and rouge is used instead now.

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

p91 added a commit to p91/code-highlighter that referenced this pull request Feb 27, 2016

Change renderer selection
As of jekyll/jekyll#3323 jekyll has a dependency on rouge. Hence, rouge is always used even if pygments is optionally installed.
This patch reverse the loopup of the renderers (previously, first check for rouge, then pygments; now: first check fir pygments, then rouge) so pygments can be used if desired.

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