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

bump rouge to 2.0.x #5230

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@Crunch09
Member

Crunch09 commented Aug 11, 2016

Hi,

this bumps rouge to 2.0.x as discussed in #4891 (comment) .

I used the HTMLLegacy formatter to avoid breaking changes but there are some minor ones as you can see in the tests (no text-align: right; on the td of the line number for example). If this is not okay, i think we would have to write our own formatter.

@@ -34,7 +34,7 @@ Gem::Specification.new do |s|
s.add_runtime_dependency('mercenary', '~> 0.3.3')
s.add_runtime_dependency('safe_yaml', '~> 1.0')
s.add_runtime_dependency('colorator', '~> 1.0')
s.add_runtime_dependency('rouge', '~> 1.7')
s.add_runtime_dependency('rouge', '~> 2.0.5')

This comment has been minimized.

@Crunch09

Crunch09 Aug 11, 2016

Member

Not sure here if we should choose ~> 2.0.5 or ~> 2.0

@Crunch09

Crunch09 Aug 11, 2016

Member

Not sure here if we should choose ~> 2.0.5 or ~> 2.0

This comment has been minimized.

@envygeeks

envygeeks Aug 11, 2016

Contributor

s.add_runtime_dependency(">= 1.7", "< 3") == "1.x", "2.x"

@envygeeks

envygeeks Aug 11, 2016

Contributor

s.add_runtime_dependency(">= 1.7", "< 3") == "1.x", "2.x"

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 11, 2016

Contributor

I'm sorry but this pull in it's current form is unacceptable for 3.x. You _must_ allow 1.x (Rouge) to remain and work around Rouges changes in order for this to be accepted in 3.x as it's a breaking change that affects people, plugins and other stuff. This is what I had tried to imply was the problem on talk.jekyllrb.com, it's just too much work, without a wrapper of which we should have built long ago.

Contributor

envygeeks commented Aug 11, 2016

I'm sorry but this pull in it's current form is unacceptable for 3.x. You _must_ allow 1.x (Rouge) to remain and work around Rouges changes in order for this to be accepted in 3.x as it's a breaking change that affects people, plugins and other stuff. This is what I had tried to imply was the problem on talk.jekyllrb.com, it's just too much work, without a wrapper of which we should have built long ago.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 11, 2016

Contributor

Either way, if you are willing to fix the problem so that both 1.x and 2.x work then it can be accepted in 3.x but until then it has a 👎 from me, because I do not wish to deal with upgrading the hundreds of plugins we have on our own sites that use Rouge 1.x in some way before absolutely necessary and even if I do, I do not wish to have to do it overnight because Jekyll decided to randomly upgrade a major dependency.

Contributor

envygeeks commented Aug 11, 2016

Either way, if you are willing to fix the problem so that both 1.x and 2.x work then it can be accepted in 3.x but until then it has a 👎 from me, because I do not wish to deal with upgrading the hundreds of plugins we have on our own sites that use Rouge 1.x in some way before absolutely necessary and even if I do, I do not wish to have to do it overnight because Jekyll decided to randomly upgrade a major dependency.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 11, 2016

Member

@envygeeks Point taken – I proposed we do this for 3.3 after it was shot down for 3.2 due to timing (#4891 (comment)). I'm 👍 on doing this if it's backwards-compatible. Could we check Rouge::VERSION?

Member

parkr commented Aug 11, 2016

@envygeeks Point taken – I proposed we do this for 3.3 after it was shot down for 3.2 due to timing (#4891 (comment)). I'm 👍 on doing this if it's backwards-compatible. Could we check Rouge::VERSION?

@parkr parkr added the enhancement label Aug 11, 2016

@parkr parkr added this to the undetermined milestone Aug 11, 2016

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Aug 12, 2016

Member

Thanks for the feedback! Will try to make it work with rouge 1.x

Member

Crunch09 commented Aug 12, 2016

Thanks for the feedback! Will try to make it work with rouge 1.x

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Aug 17, 2016

Member

Making some good progress on this!
But i am not sure which Formatter we should choose for 2.x ? I guess we don't need to use the Rouge::Formatters::HTMLLegacy formatter because users who want this behaviour are still free to use Rouge ~> 1.7.

Member

Crunch09 commented Aug 17, 2016

Making some good progress on this!
But i am not sure which Formatter we should choose for 2.x ? I guess we don't need to use the Rouge::Formatters::HTMLLegacy formatter because users who want this behaviour are still free to use Rouge ~> 1.7.

%(</tr></tbody></table></code></pre></figure>\n\n) +
%(<p>This should not be highlighted, right?</p>),
@result
)

This comment has been minimized.

@pathawks

pathawks Oct 19, 2016

Member

This was much easier to read before. Is there a reason this needed to be changed?

@pathawks

pathawks Oct 19, 2016

Member

This was much easier to read before. Is there a reason this needed to be changed?

This comment has been minimized.

@Crunch09

Crunch09 Oct 19, 2016

Member

The only reason here was to be consistent with the test on line 318.

@Crunch09

Crunch09 Oct 19, 2016

Member

The only reason here was to be consistent with the test on line 318.

@crispgm crispgm referenced this pull request Nov 7, 2016

Closed

Upgrade to Rouge 2.0 #5556

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Nov 7, 2016

Member

Fixes #5556

Member

pathawks commented Nov 7, 2016

Fixes #5556

@jneen

This comment has been minimized.

Show comment
Hide comment
@jneen

jneen Nov 7, 2016

Hi! What I recommend doing is actually defining your own formatter, especially if there are special things you want it to do. You can look through the existing formatters as well as the Gitlab Formatter. Alternately, you can use Formatters::HTML to generate plain unadorned HTML and add stuff around it as you like. Looking at the code you have, you might also consider using Formatters::HTMLTable, which takes an inner formatter (Formatters::HTML in your case) and a number of line numbering options.

jneen commented Nov 7, 2016

Hi! What I recommend doing is actually defining your own formatter, especially if there are special things you want it to do. You can look through the existing formatters as well as the Gitlab Formatter. Alternately, you can use Formatters::HTML to generate plain unadorned HTML and add stuff around it as you like. Looking at the code you have, you might also consider using Formatters::HTMLTable, which takes an inner formatter (Formatters::HTML in your case) and a number of line numbering options.

@jneen

This comment has been minimized.

Show comment
Hide comment
@jneen

jneen Nov 7, 2016

Ah yes, and if you want to support both 1.x and 2.x, you'll have to define a wrapper, as the APIs are different.

jneen commented Nov 7, 2016

Ah yes, and if you want to support both 1.x and 2.x, you'll have to define a wrapper, as the APIs are different.

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Nov 7, 2016

Member

@jneen Thank you!

Member

Crunch09 commented Nov 7, 2016

@jneen Thank you!

@fliiiix

This comment has been minimized.

Show comment
Hide comment
@fliiiix

fliiiix Nov 26, 2016

@Crunch09 are you working on it?

fliiiix commented Nov 26, 2016

@Crunch09 are you working on it?

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Nov 26, 2016

Member

@fliiiix Yes. Sorry for the delay, it's been a busy couple of weeks but there should be an update by the end of the weekend.

Member

Crunch09 commented Nov 26, 2016

@fliiiix Yes. Sorry for the delay, it's been a busy couple of weeks but there should be an update by the end of the weekend.

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Nov 27, 2016

Member

@jekyll/core Unfortunately i still have some open questions regarding this and i hope you can help to clear them up so i can push an update:

  • If we write our own formatter, should it produce the exact same output as now with Rouge 1.7? If not, what should / is allowed to change compared to the current version? I guess if we were to change some things this wouldn't be a breaking change for users as they still could use the old Rouge version.
  • If we want to land this in jekyll 3.4 and support both Rouge versions should there be different CI builds to test against each version?
Member

Crunch09 commented Nov 27, 2016

@jekyll/core Unfortunately i still have some open questions regarding this and i hope you can help to clear them up so i can push an update:

  • If we write our own formatter, should it produce the exact same output as now with Rouge 1.7? If not, what should / is allowed to change compared to the current version? I guess if we were to change some things this wouldn't be a breaking change for users as they still could use the old Rouge version.
  • If we want to land this in jekyll 3.4 and support both Rouge versions should there be different CI builds to test against each version?
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 16, 2017

Member

Fixes #5774.

Member

parkr commented Jan 16, 2017

Fixes #5774.

@willclarktech

This comment has been minimized.

Show comment
Hide comment
@willclarktech

willclarktech May 27, 2017

Hi, is there any movement on this? Currently code highlighting of ES6 template literals are broken on GitHub Pages sites because rouge 1.x doesn’t support them.

willclarktech commented May 27, 2017

Hi, is there any movement on this? Currently code highlighting of ES6 template literals are broken on GitHub Pages sites because rouge 1.x doesn’t support them.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jun 13, 2017

Member

If we write our own formatter, should it produce the exact same output as now with Rouge 1.7? If not, what should / is allowed to change compared to the current version? I guess if we were to change some things this wouldn't be a breaking change for users as they still could use the old Rouge version.

If a user has Rouge 1.x installed, the output should be unchanged. If a user has Rouge 2.x installed, they should be aware that the output might be different.

If we want to land this in jekyll 3.4 and support both Rouge versions should there be different CI builds to test against each version?

I don't think we need to double the size of our matrix, but we should have at least one that tests with the old Rouge and at least one that tests with the new. Probably only have one that tests against Rouge 1.x and move everything else to 2.x?

Member

pathawks commented Jun 13, 2017

If we write our own formatter, should it produce the exact same output as now with Rouge 1.7? If not, what should / is allowed to change compared to the current version? I guess if we were to change some things this wouldn't be a breaking change for users as they still could use the old Rouge version.

If a user has Rouge 1.x installed, the output should be unchanged. If a user has Rouge 2.x installed, they should be aware that the output might be different.

If we want to land this in jekyll 3.4 and support both Rouge versions should there be different CI builds to test against each version?

I don't think we need to double the size of our matrix, but we should have at least one that tests with the old Rouge and at least one that tests with the new. Probably only have one that tests against Rouge 1.x and move everything else to 2.x?

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Jun 14, 2017

Member

@pathawks Thank you for the clarification, that should help!

Member

Crunch09 commented Jun 14, 2017

@pathawks Thank you for the clarification, that should help!

@guymorita

This comment has been minimized.

Show comment
Hide comment
@guymorita

guymorita Jun 17, 2017

Can we get this merged in? Would help lots. Thanks.

guymorita commented Jun 17, 2017

Can we get this merged in? Would help lots. Thanks.

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Jun 18, 2017

Member

@guymorita Hi, please see #6150 for updates.

Member

Crunch09 commented Jun 18, 2017

@guymorita Hi, please see #6150 for updates.

@Crunch09 Crunch09 closed this Jun 18, 2017

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