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

[WIP] Add shim that works for both Rouge 1 and Rouge 2 #5919

Merged
merged 8 commits into from Aug 17, 2017
Merged

Conversation

@parkr
Copy link
Member

parkr commented Mar 2, 2017

This PR aims to allow Rouge v1 or v2 to be used with Jekyll. This will allow users to choose to stay on the old version or to upgrade if possible/they want to.

In #5230, we learned from @jneen that we cannot rely on the same classes if folks want to upgrade to Rouge 2.

I have attempted to create a helper utility which creates the necessary Rouge formatter that we need to process our files.

While this is still a work in progress, I would appreciate any general opinions on the API design. I think I may have misinterpreted @jneen's comments in #5230.

/cc github/pages#1359

Crunch09 and others added 3 commits Aug 11, 2016
@parkr parkr added the enhancement label Mar 2, 2017
@DirtyF

This comment has been minimized.

Copy link
Member

DirtyF commented Mar 2, 2017

I made some quick testing:

Given I upgrade Jekyll's repository to rouge 2.0.7
Then code snippets are not properly rendered and<pre class="highlight"><code> ... </code></pre> are missing

@Crunch09

This comment has been minimized.

Copy link
Member

Crunch09 commented Mar 2, 2017

Hey 👋 the main issue i encountered when working on #5230 was that it didn't seem possible to rebuilt the exact same output we had when using Rouge 1.7 and i was also not sure we even want to have the same exact output, that's why i couldn't continue (#5230 (comment)).

In my opinion it would be best to have the input of some users who actually want to use the new features of Rouge 2 with Jekyll and then decide on a custom formatter (based on a provided formatter from Rouge v2 as @jneen suggested).

@parkr Still happy to help if you need any form of support on implementing this

@pathawks pathawks mentioned this pull request Mar 26, 2017
@parkr

This comment has been minimized.

Copy link
Member Author

parkr commented Mar 31, 2017

In my opinion it would be best to have the input of some users who actually want to use the new features of Rouge 2 with Jekyll and then decide on a custom formatter (based on a provided formatter from Rouge v2 as @jneen suggested).

Thanks! So it sounds like all people want from Rouge v2 are the extra formatters – the Ruby API doesn't really matter. The reason we can't upgrade is that some people have written their own formatters so we can't block them from using Rouge 1.

I wonder: if they're writing their own formatters or monkey-patching Jekyll to modify how it uses Rouge, then maybe we can get away with allowing Rouge 1 & 2 in the gemspec, upgrading to Rouge 2 in our code?

@envygeeks I seem to remember you mentioning that you'd done some customization with Rouge. Can you help us understand what changes we can make to allow Rouge 2 that wouldn't affect you? If you're monkey-patching, for example, then we could just change the methods you have money-patched and be totally in the clear as long as our gemspec allows either version.

@nickbabcock nickbabcock mentioned this pull request Apr 10, 2017
@allanbowe allanbowe mentioned this pull request May 20, 2017
@allanbowe

This comment has been minimized.

Copy link

allanbowe commented Jun 6, 2017

We are currently developing a lexer as a contribution to Rouge, which we'd like to then use automatically on github pages, HOWEVER - it appears that this development will be in vain unless this pull request is merged.

What are the blockers?

@Crunch09

This comment has been minimized.

Copy link
Member

Crunch09 commented Jun 6, 2017

@allanbowe Hey, you can see the main issues mentioned in the posts above.

Seeing as we don't make progress with this as it is right now, i'd propose we settle for one custom formatter (while still supporting Rouge 1) and allow users to use their own formatter. Based on hopefully more feedback once this is out we can then improve the new formatter.

@jneen

This comment has been minimized.

Copy link

jneen commented Jun 13, 2017

Hi! This has gone on for a really long time. Rouge 2.0 was released a year ago, and 1.x is no longer supported. I'm getting issues opened on Rouge about bugs that were fixed ages ago because Jekyll is still on an ancient version :\

cf. github/pages-gem#436 rouge-ruby/rouge#698

cc @gfx @dblessing @selivan

@jneen

This comment has been minimized.

Copy link

jneen commented Jun 13, 2017

@Crunch09 It is possible to write a wrapper class that uses either Rouge::Formatters::HTML or Rouge::Formatters::HTMLLegacy depending on the existence of the latter, which exists in 2.x and not in 1.x. Then Jekyll should be able to unpin its rouge version number and allow it to be selected downstream.

@jneen

This comment has been minimized.

Copy link

jneen commented Jun 13, 2017

Also, I'm not clear on the incompatibility story for users who wrote their own formatters - if they wrote their own, how are they affected by the existing ones changing?

@Crunch09

This comment has been minimized.

Copy link
Member

Crunch09 commented Jun 14, 2017

@jneen Thank you for your reply! I'm not a maintainer for Jekyll though, so i think it would be best if @parkr could maybe list what needs to be done before we can release it?
Otherwise i'd be happy to continue on the weekend on either #5230 or help with this PR based on your comments here and @pathawks comment in #5230.

@jneen

This comment has been minimized.

Copy link

jneen commented Jun 14, 2017

I think the best approach is to use HTMLLegacy if it exists, and otherwise fall back to HTML. They should have the same behavior. Following that, I would unpin the rouge version in the gemspec, since this gem would then work for any version of rouge.

@DirtyF DirtyF added this to the 3.6 milestone Jun 18, 2017
@parkr

This comment has been minimized.

Copy link
Member Author

parkr commented Jun 18, 2017

#6150 supersedes this.

@parkr parkr closed this Jun 18, 2017
@parkr parkr reopened this Jun 18, 2017
@parkr

This comment has been minimized.

Copy link
Member Author

parkr commented Jun 18, 2017

#6150 points to this PR.

Crunch09 and others added 2 commits Jun 20, 2017
* use `Rouge::Formatters::HTMLLegacy` and fix tests

* set default rouge version to 2.1.0

* allow more than just 2.1.x versions by default
@DirtyF

This comment has been minimized.

Copy link
Member

DirtyF commented Jun 21, 2017

@Crunch09 I have a hard time understanding why HTMLLegacy does not generate the same output as in Rouge 1.x.

@parkr

This comment has been minimized.

Copy link
Member Author

parkr commented Jun 21, 2017

My understanding is that HTMLLegacy simply provides the same Ruby API, but it uses the new HTML-based formatters internally.

@Crunch09

This comment has been minimized.

Copy link
Member

Crunch09 commented Jun 21, 2017

@DirtyF I think @parkr is right. From the rouge readme:

HTMLLegacy is a backwards-compatibility class intended for users of rouge 1.x,
with options that were supported then.

If users want to be sure there are absolutely zero breaking changes in their site they can (according to this PR) still use the old rouge version with upcoming minor versions of jekyll.
If they want to use the new rouge version instead, there might be some changes but IMHO that should be expected if they do a major version update.

@parkr

This comment has been minimized.

Copy link
Member Author

parkr commented Jun 22, 2017

Just tested on Jekyll's own website, <pre class="highlight"><code> ... </code></pre> markup is still missing, so it breaks the rendering as you can see in the screenshot

I'm still concerned about this ☝️

@pathawks

This comment has been minimized.

Copy link
Member

pathawks commented Jun 22, 2017

Just tested on Jekyll's own website, <pre class="highlight"><code> ... </code></pre> markup is still missing, so it breaks the rendering as you can see in the screenshot

I don't really understand what this has to do with Rouge; isn't that stuff added on our end anyway?

@DirtyF

This comment has been minimized.

Copy link
Member

DirtyF commented Jun 22, 2017

@pathawks I didn't know that!

@Crunch09

This comment has been minimized.

Copy link
Member

Crunch09 commented Jun 22, 2017

Could you please try it again with rouge 2.1.1 @DirtyF which has been released today? It seems to have fixed the issue for me locally.

@DirtyF

This comment has been minimized.

Copy link
Member

DirtyF commented Jun 22, 2017

@Crunch09 tested this branch on two different websites (jekyll docs and one of mine) and still no <pre> and <code> tags in the generated HTML, as a result code blocks are not properly rendered.

@pathawks

This comment has been minimized.

Copy link
Member

pathawks commented Jun 22, 2017

@DirtyF Odd that the test is passing, but it isn't working for you.

@Crunch09

This comment has been minimized.

Copy link
Member

Crunch09 commented Jun 22, 2017

@DirtyF Sorry, my mistake. Ran it without bundle exec so was not using this branch :( Also still failing for me.
@pathawks I think the code you posted will only be added when using the highlight tag. The mentioned code is only a raw tag though.

@DirtyF

This comment has been minimized.

Copy link
Member

DirtyF commented Jun 23, 2017

Odd that the test is passing

yeah maybe we don't have the proper rendering tests here?
On the other site I tested, the source is a pure Markdown code block, no rawor highlightLiquid tags are involved.

@Crunch09

This comment has been minimized.

Copy link
Member

Crunch09 commented Jun 24, 2017

@DirtyF Found the issue. (This time hopefully for real 🙄 )
TL;DR: The kramdown parser is not yet released in a Rouge 2 compatible form.

Support for Rouge 2 has been added but is yet to be released.
Example:

```liquid
{% raw %}
{{ site.baseurl }}
{% endraw %}
```

Produces in Rouge 1.11 with the current kramdown release 1.13.2:

<div class="language-liquid highlighter-rouge"><pre class="highlight"><code>
<span class="p">{{ </span><!-- ... --> %}</span>
</code></pre>
</div>

Produces in Rouge 2.10 with the current kramdown release 1.13.2 (as noted by DirtyF):

<div class="language-liquid highlighter-rouge">
<span class="p">{{ </span><!-- ... --> %}</span>
</div>

Produces in Rouge 2.10 with the unreleased kramdown version (current master):

<div class="language-liquid highlighter-rouge"><div class="highlight"><pre class="highlight"><code>
<span class="p">{{ </span><!-- ... --> %}</span>
</code></pre></div></div>

The last code block adds an additional div compared to Rouge 1.11 but it will look the same as in the old Rouge version.

To try it out locally (with jekyll docs):

  • The kramdown repo has to be cloned. The version on github can't be used directly in the Gemfile as it doesn't include a .gemspec.
  • After cloning run:
rake install
rake gemspec 
  • Now reference the local version in Jekyll's Gemfile and run bundle
  • Remove {% feed_meta %} from docs/_includes/top.html, otherwise the docs site will not built. (still investigating why)
  • Serve the docs

I could submit a failing PR for this behavior but not sure it'll be worth it as it'll only pass once a new kramdown version has been released.

@Crunch09

This comment has been minimized.

Copy link
Member

Crunch09 commented Jun 24, 2017

Asked now if we could have a new kramdown release: gettalong/kramdown#439

@pathawks

This comment has been minimized.

Copy link
Member

pathawks commented Jun 26, 2017

I could have sworn that at one point we were overriding kramdown's built-in highlighting so that it would be as if the user had used the {% highlight %} tag instead. Fever dream?

@Crunch09

This comment has been minimized.

Copy link
Member

Crunch09 commented Jul 18, 2017

I don't know what the release plan for jekyll is but given that 3.5.1 has been released and this is in the 3.6 milestone and all tests are passing, would it make sense to merge this into master?
My assumption is that at least some people would test their site with the latest master regularly and we might get helpful feedback on the rouge 2 integration. What do you think?

@DirtyF

This comment has been minimized.

Copy link
Member

DirtyF commented Jul 18, 2017

@Crunch09 I'm all for it, just waitin' on @parkr's call on this.

@parkr parkr mentioned this pull request Aug 17, 2017
3 of 3 tasks complete
@parkr

This comment has been minimized.

Copy link
Member Author

parkr commented Aug 17, 2017

This is the main drive for v3.6.0. I've branched off a 3.5-stable so we can do patch releases for 3.5 so master is now for 3.6. Let's get this merged.

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 4c15b9e into master Aug 17, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jekyllbot jekyllbot deleted the rouge-1-and-2 branch Aug 17, 2017
jekyllbot added a commit that referenced this pull request Aug 17, 2017
parkr added a commit that referenced this pull request Aug 17, 2017
@jekyll jekyll locked and limited conversation to collaborators Jul 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.