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

Drop support for pygments as syntax-highlighter #6983

Closed
wants to merge 8 commits into from

Conversation

ashmaroli
Copy link
Member

@ashmaroli ashmaroli commented May 3, 2018

Resolves #6823

  • stop testing output from the use of Pygments as syntax highlighter
  • maintain API for handling Pygments so that a plugin can easily hook into, if needed in future.
    e.g. The plugin would just have to override HighlightBlock::render_pygments to provide the functionality.

@ashmaroli ashmaroli added the breaking-change Affect current behavior label May 3, 2018
@ashmaroli ashmaroli added this to the 4.0 milestone May 3, 2018
@ashmaroli ashmaroli requested a review from a team May 3, 2018 17:27
highlighted_code.sub('<div class="highlight"><pre>', "").sub("</pre></div>", "")
def render_pygments(code, _is_safe)
Jekyll.logger.warn "Warning:",
"Highlight Tag no longer supports rendering with Pygments"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty clever idea 💡👍🏼

@crispgm
Copy link
Member

crispgm commented May 4, 2018

👍And do u forget the Gemfile?

@ashmaroli
Copy link
Member Author

..u forget the Gemfile?

@crispgm Ah yes, I did..!! Thanks 👍

@DirtyF
Copy link
Member

DirtyF commented May 4, 2018

Our docs should reflect that change https://jekyllrb.com/docs/templates/#code-snippet-highlighting
There's also a mention in a tutorial: https://jekyllrb.com/tutorials/orderofinterpretation/#markdown-in-include-file-not-processed

@ashmaroli
Copy link
Member Author

Our docs should reflect that change

I left that out intentionally since our docs are not versioned.. I believe that a 4.0 change should not be included in the current docs.

@DirtyF
Copy link
Member

DirtyF commented May 4, 2018

@ashmaroli right, it was more of a reminder. Maybe we could keep a list of changes to make to the docs before the release instead?

@ashmaroli
Copy link
Member Author

Maybe we could keep a list of changes to make to the docs before the release instead?

Yes. Exactly what I was thinking..
My plan currently is as follows:

  • Create branch docs-40
  • Update docs as required either by us (or the contributor theirself)
  • merge into release-4.0 when its time

@pathawks
Copy link
Member

pathawks commented May 4, 2018

Maybe it would be best if we didn't work on 4.0 on master. That way, we could continue to update docs right along changes to code.

Or, we could create a new branch for the live site. GitHub Pages would build from that branch, and master would get all the 4.0 docs changes. Then when 4.0 is released, we point GitHub Pages back to master and delete the other branch.

@ashmaroli
Copy link
Member Author

I feel like I've encountered a stalemate..

Maybe it would be best if we didn't work on 4.0 on master

Now that we've started working on 4.0 I no longer feel the benefits of working on a 4.0-dev especially since JekyllBot commits PR merges to master. Its probably only going to be an additional overhead on the maintainers..

Or, we could create a new branch for the live site

This is an additional overhead for us to maintain a temporary site source on the gh-pages branch with no significant gain..

@pathawks
Copy link
Member

pathawks commented May 4, 2018

Or, we could create a new branch for the live site
This is an additional overhead for us to maintain a temporary site source on the gh-pages branch with no significant gain..

I worry that having a bunch of open docs PRs that must be kept up-to-date with code changes and that must be kept relatively free of merge conflicts and that cannot be merged until the day 4.0 launches would be far more overhead.

@ashmaroli
Copy link
Member Author

that must be kept up-to-date with code changes and that must be kept relatively free of merge conflicts and that cannot be merged until the day 4.0 launches would be far more overhead.

True.. true.. true..

@DirtyF DirtyF added this to Confirmed in Jekyll 4.1 May 7, 2018
@DirtyF DirtyF moved this from Confirmed to In progress in Jekyll 4.1 May 7, 2018
@ashmaroli ashmaroli moved this from In progress to Reviewable in Jekyll 4.1 May 8, 2018

highlighted_code.sub('<div class="highlight"><pre>', "").sub("</pre></div>", "")
def render_pygments(code, _context)
Jekyll.logger.warn "Warning:",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably fail the build, right?

Otherwise, folks who use (ie) GitHub Pages will just stop having syntax highlighting in their code with no clue as to why, and that is if they notice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah.. that's true..
I'd prefer users to keep abreast with changes in a major version and check for any warnings in their terminal output instead of getting a Page Build Error email from GitHub Pages..
but that's just me..

People just don't have the time to do a local run first.. 🙂
I'll update it to raise instead

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pathawks Or... should we silently fallback to highlighting with Rouge? 💡

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashmaroli if there is no compatibility issue between the two highlighters, this could be the nicest solution for all users.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the README at jneen/rouge:

Its HTML output is compatible with stylesheets designed for pygments.

If there's still a need for using pygments explicitly, the community can build a plugin which will be able to easily hook into the render method here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rouge::Formatters::HTMLPygments.new(formatter, css_class='codehilite') wraps the given formatter with div wrappers generally expected by stylesheets designed for Pygments

@ashmaroli
Copy link
Member Author

@oe @pathawks Any updates on this..?

@pathawks pathawks requested a review from a user June 20, 2018 16:21
@pathawks
Copy link
Member

pathawks commented Jul 8, 2018

Where are the changes to documentation? How did we decide to handle that?
Do we need a separate PR for that, and maybe a 4.0-docs milestone or something similar?

@ghost
Copy link

ghost commented Jul 8, 2018

We could hide 4.0-specific doc changes behind a flag that we enable once the release has been done? I'm also not too into the idea of keeping a whole separate branch or lots of open PRs.

@ashmaroli
Copy link
Member Author

ashmaroli commented Jul 9, 2018

hide 4.0-specific doc changes behind a flag that we enable once the release has been done

@oe Such a flag doesn't currently exist.
One solution is use Liquid to output 4.0 relevant changes only when site.version >= 4.0

{% if site.version >= 4.0.0 %}
  Documentation for Jekyll 4.0
{% else %}
  Existing documentation
{% endif %}

/cc @DirtyF what say?

@ashmaroli
Copy link
Member Author

This PR seems to gathered a lot of noise. I'm going to close this in favor of a new one.

@ashmaroli ashmaroli closed this Jul 9, 2018
@ashmaroli ashmaroli removed this from the 4.0 milestone Jul 9, 2018
@jekyll jekyll locked and limited conversation to collaborators Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Jekyll 4.1
  
Reviewable
Development

Successfully merging this pull request may close these issues.

Drop Pygments
5 participants