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

Improve support for Rouge 1.x and 2.x #880

Merged
merged 9 commits into from Nov 15, 2016

Conversation

Projects
None yet
5 participants
@barraq
Copy link
Contributor

commented Jun 29, 2016

This commit improves support for Rouge 1.x and adds support for Rouge 2.x.

  • Support for following options are introduced:
    • inline_theme (default: nil)
    • line_numbers (default: false)
    • start_line (default: 1)
    • wrap (default: false)
  • Support for formatters (including HTMLLegacy, HTMLInline, HTMLLinewise, HTMLPygments, HTMLTable) is introduced for Rouge 2.x

Todo:

  • Initial (might do better) implementation
  • Split colorize filter tests
  • Introduce Appraisal gem
  • Setup Appraisal for Rouge 1.x and 2.x
  • Update Travis to run test for both Rouge 1.x and Rouge 2.x
  • Refactor Rouge tests to specs
  • Add specific tests for Rouge 2.x regarding HTMLInline
  • Add specific tests for Rouge 2.x regarding HTMLLinewise
  • Add specific tests for Rouge 2.x regarding HTMLTable
  • Add specific tests for Rouge 2.x regarding HTMLPygments
  • Simplified Rouge 2.x implementation (allow any kind of formatters)
  • Improve Rake/Travis configuration to only run Rouge specs for Rouge appraisal

Usage for Rouge 1.x (no change):

filter :colorize_syntax,
       :default_colorizer => :rouge,
       :rouge => { line_numbers: true, wrap: true, css_class: 'highlight' }

Usage for Rouge 2.x:

# Legacy support
filter :colorize_syntax,
       :default_colorizer => :rouge,
       :rouge => { legacy: true, line_numbers: true, wrap: true, css_class: 'highlight' }

# Custom formatter (here HTML Inline)
filter :colorize_syntax,
       :default_colorizer => :rouge,
       :rouge => { formatters: Rouge::Formatters::HTMLInline.new(MY_THEME) }

See Rouge 2.x doc for other formatters

css_class: params.fetch(:css_class, 'highlight'),
}
formatter = Rouge::Formatters::HTML.new(formatter_options)
if Rouge.version < '2' || params.fetch(:legacy, false)

This comment has been minimized.

Copy link
@barraq

barraq Jun 29, 2016

Author Contributor

we might change the logic here so that if we have Rouge 2.x and than neither inline, linewise, pygments, table is define and than there is arguments given then we fallback to legacy. That would prevent from explicitly specifying legacy.

@barraq barraq force-pushed the barraq:feature/improve-rouge-support branch from 2723bdd to efaa344 Aug 21, 2016

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Aug 22, 2016

@barraq What is the status of this PR?

@ddfreyne ddfreyne changed the title [WIP] Improve support for Rouge 1.x and 2.x Improve support for Rouge 1.x and 2.x Aug 22, 2016

@barraq

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2016

@ddfreyne I didn't have the time to go over this Travis things to run test for both Rouge 1.x and Rouge 2.x. If you know how to do it quick it would be cool. I can add the missing tests then.

@connorshea

This comment has been minimized.

Copy link

commented Oct 11, 2016

You can use separate gemfiles that use Rouge 1.x vs. 2.x, see the docs.

Personally I'd prefer an approach that just uses the bundle install --gemfile approach so you can switch to a different CI system more easily, but that's not really necessary.

For a .travis.yml example see carrierwave.

@barraq

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2016

Thanks @connorshea. Ok @ddfreyne like discussed I am looking at https://github.com/thoughtbot/appraisal

@barraq barraq force-pushed the barraq:feature/improve-rouge-support branch 4 times, most recently from 223de1a to 3d1c223 Oct 17, 2016

@barraq barraq force-pushed the barraq:feature/improve-rouge-support branch 2 times, most recently from f2083a9 to af37a8c Oct 25, 2016

@barraq

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2016

@ddfreyne I am almost done here, any comments?

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Oct 29, 2016

This is great work!

I suppose the only thing that’s left is to only run the Rouge specs twice (rather than everything).

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Oct 29, 2016

Question: How are the gemfiles/rouge_1.gemfile and gemfiles/rouge_2.gemfile files generated? Can they be auto-generated, or should they be checked in?

@barraq

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2016

Thanks @ddfreyne, regarding gemfiles/* they are generated by Appraisal by doing appraisal install (see https://github.com/thoughtbot/appraisal#under-the-hood) So they are autogenerated and are based on the initial Gemfile.

@barraq barraq force-pushed the barraq:feature/improve-rouge-support branch 5 times, most recently from 94bd69d to 31805a8 Oct 30, 2016

@barraq
Copy link
Contributor Author

left a comment

@ddfreyne please let me know what you think of this approach ;)

@@ -0,0 +1,35 @@
class Nanoc::Filters::ColorizeSyntax::PigmentizeTest < Nanoc::TestCase

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Oct 30, 2016

Member

Pygmentize, not pigmentize (the “Py” is from Python!)

Same for the filename.

@@ -0,0 +1,62 @@
# This file was generated by Appraisal

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Oct 30, 2016

Member

Are these gemfiles still necessary, given that appraisal install is executed already?

This comment has been minimized.

Copy link
@barraq

barraq Oct 30, 2016

Author Contributor

True, actually those gemfiles are only needed when we want to run all the tests. Since in our case we only wants to run focus spec we can remove them.

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Oct 30, 2016

Looks great! Just some minor comments.

@barraq barraq force-pushed the barraq:feature/improve-rouge-support branch 2 times, most recently from b83e4d3 to 66aa197 Oct 30, 2016

barraq added some commits Jun 27, 2016

Improve support for Rouge 1.x and 2.x
This commit improves support for Rouge 1.x and adds support for Rouge 2.x.
- Support for following options are introduced:
  - inline_theme (default: nil)
  - line_numbers (default: false)
  - start_line (default: 1)
  - wrap (default: false)
- Support for HTMLLegacy, HTMLInline, HTMLLinewise, HTMLPygments, HTMLTable is introduced for Rouge 2.x
Setup travis to run tests for Rouge 1.x and 2.x
Note that all tests are run twice. As further improvement we want to run
only Rouge tests for each Rouge version. All other tests should only
be run with default project Gemfile.
Extend specs for Rouge 2.x
- Add HTMLInline specs
- Add HTMLLinewise specs
- Add HTMLTable specs
- Add HTMLPygments specs

@barraq barraq force-pushed the barraq:feature/improve-rouge-support branch from 66aa197 to f693b56 Nov 1, 2016

@barraq

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2016

@ddfreyne ready to merge?

@ddfreyne ddfreyne added this to the 4.3.8 milestone Nov 15, 2016

@ddfreyne ddfreyne merged commit 4802d79 into nanoc:master Nov 15, 2016

1 check passed

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

This comment has been minimized.

Copy link
Member

commented Nov 15, 2016

Done. Thank you for your effort!

@whitequark

This comment has been minimized.

Copy link
Member

commented Nov 15, 2016

\o/

@axilleas

This comment has been minimized.

Copy link

commented Nov 18, 2016

Awesome! Does that mean 4.3.8 is about to be published? 😄

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Nov 18, 2016

Soon!

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Nov 18, 2016

Done!

@connorshea

This comment has been minimized.

Copy link

commented Nov 18, 2016

@ddfreyne @barraq thank you! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.