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

resolves #261 use the new syntax highlighter API #287

Merged

Conversation

ggrossetie
Copy link
Member

This new API was introduced in Asciidoctor 2.0.0.
If the syntax_highlighter method is not available fallback to the "legacy" syntax highlighting.

When using the new syntax highlighting API, code blocks are encapsulated in a <div class="listingblock"> and <div class="content">. I think it might be possible to remove them using the "transform" option on the pre and code elements to add the missing classes/attributes.

Not sure if the "noescape" data attribute is mandatory or not but when the syntax highlighter is "highlight.js", the code element will have the following attribute data-noescape="true".

I think we should release a version 3.0.0 where we break compatibility with Asciidoctor < 2.0.0 because:

  • we only test against the latest version of Asciidoctor
  • the code is "duplicated" because we use two distinct approaches to do syntax highlighting
  • it will reduce the maintenance

@obilodeau
Copy link
Member

I think we should release a version 3.0.0 where we break compatibility with Asciidoctor < 2.0.0

Agreed. Please drop the legacy syntax highlighting support from this PR.

Otherwise the generated files are suffixed with .revealjs when using bundle exec rake examples:convert
@ggrossetie ggrossetie force-pushed the issue-261-syntax-highlighter-api-2-0 branch 3 times, most recently from 5cbb4f0 to 43ef818 Compare December 10, 2019 08:15
@ggrossetie ggrossetie force-pushed the issue-261-syntax-highlighter-api-2-0 branch from 43ef818 to 464263e Compare December 10, 2019 08:30
@obilodeau
Copy link
Member

Is it possible that pygments doesn't work under JRuby? If all the builds are going to fail after this is merged I would rather disable the pygments tests and document it. The asciidoctor docker image did something similar recently: asciidoctor/docker-asciidoctor#63

@ggrossetie
Copy link
Member Author

It's somehow working on Asciidoctor:

I think the reason is that we are using Ubuntu xenial (with Python 3 I assume) and Asciidoctor is using Ubuntu trusty (with Python 2).
But I'm not sure why it's working with MRI... I might be wrong though...

If it's only failing in JRuby, I guess we could add a condition in the Gemfile:

group :development do
  gem 'pygments.rb' unless RUBY_ENGINE == 'jruby'
  gem 'rouge'
end

Or just remove the dependency on Pygments?

At this point, we're recommending against the use of pygments as the source highlighter in Asciidoctor. Rouge is now supported by default in the major converters and doesn't rely on a system command or separate language runtime. So unless there is really good reason to continue supporting pygments in the Docker image, I'd make the argument that we should just drop it.

asciidoctor/docker-asciidoctor#63 (comment)

@obilodeau
Copy link
Member

I don't want to get rid of it since it does seem to work with MRI but I don't want it to make Travis fail since it's in bad shape (but that could be fixed by upstream eventually).

If it's only failing in JRuby, I guess we could add a condition in the Gemfile
Unless I'm mistaken, only doing this is not going to make tests pass. That plus disabling the pygments doctest would probably make it happy.

If disabling doctest per engine is easy let's do that. Otherwise disabling all pygments test would bother me too much.

@ggrossetie
Copy link
Member Author

@obilodeau I've added the pygments.rb dependency in the if RUBY_ENGINE != 'jruby' block.
Tests are green 🎉

@obilodeau
Copy link
Member

Converting file examples/source-pygments.adoc... asciidoctor: WARNING: optional gem 'pygments.rb' is not available (reason: cannot load 'pygments'). Functionality disabled.
✔️;49m✔️

Fair enough 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants