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.

Sign up for GitHub

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

Jump to bottom

Expand supported rouge versions #29

Merged
merged 1 commit into from Mar 12, 2023
Merged

Expand supported rouge versions #29

merged 1 commit into from Mar 12, 2023

Conversation

ashmckenzie
Copy link
Contributor

@ashmckenzie ashmckenzie commented Feb 13, 2023 •
edited

Describe the change

This PR increases the supported versions of rouge to be ">= 3.14", "< 5.0" as there are new 4.x versions of rouge.

Why are we doing this?

The changes allows projects to install this gem as well as versions 4.x of the rouge gem.

Benefits

Same as above.

Drawbacks

https://github.com/rouge-ruby/rouge/releases/tag/v4.0.0 dropped support for versions of Ruby < 2.7, so versions 2.7+ are only supported with rouge 4.x.

Requirements

  • Tests written & passing locally?
  • Code style checked?
  • Rebased with master branch?
  • Documentation updated?
  • Changelog updated? - I haven't updated this as I'm not sure if you want a new minor or patch version.

ashmckenzie
ashmckenzie commented Feb 13, 2023
View reviewed changes
tty-markdown.gemspec Outdated
@@ -29,7 +29,7 @@ Gem::Specification.new do |spec|

spec.add_dependency "kramdown", ">= 1.16.2", "< 3.0"
spec.add_dependency "pastel", "~> 0.8"
spec.add_dependency "rouge", "~> 3.14"
spec.add_dependency "rouge", ">= 3.14", "< 4.2"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

thought: I went with "< 4.2" because I tested (manually and ran the specs) against rouge 4.1.0 and it worked fine, so it should be OK to at least then.

Copy link
Owner

Choose a reason for hiding this comment

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

I'd take a leap of faith here and further relax the constraint to "< 5.0". Otherwise, this may cause a bit of churn having to nudge the upper limit up whenever new versions of 4.x are released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated 👍

@ashmckenzie
Copy link
Contributor Author

For additional context, we use https://docs.renovatebot.com/ruby/ to keep our Ruby gems up-to-date and it's automatically created an MR at https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111543 which highlights the problem I'm hoping to fix here 🙂🙏

WDYT @piotrmurach?

@piotrmurach
Copy link
Owner

Hi Ash 👋

Thanks for this PR. Agreed, let's relax the rouge constraint. This is a backwards-compatible change so we should be good to release as 0.7.2. In the changelog, please use this version but don't specify a release date. Instead, put unreleased as I'm not sure when I'll get around to releasing it.

@ashmckenzie ashmckenzie force-pushed the ashmckenzie/update-rouge-dependency branch from 3a6501a to e6ae34f Compare February 14, 2023 01:09
@ashmckenzie
Copy link
Contributor Author

Thanks for the swift reply @piotrmurach 🙇 I've updated with your suggestions 🏓

@ashmckenzie
Copy link
Contributor Author

I'm not sure what's happening with CI @piotrmurach, seems to be stuck?

@bogdanvlviv
Copy link

Looking forward to 0.7.2 release with this change. 🙇 This will allow us to update rouge gem to the latest version to fix issue with git-diff rendering in GitLab.

@ashmckenzie
Copy link
Contributor Author

@piotrmurach, is there anything we can assist with in getting this PR merged please? 🙏🙂

piotrmurach
piotrmurach requested changes Mar 6, 2023
View reviewed changes
Copy link
Owner

@piotrmurach piotrmurach left a comment

Choose a reason for hiding this comment

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

Sorry for the silence - busy times. Small nitpicky changes to the changelog.

CHANGELOG.md Outdated
@@ -1,5 +1,8 @@
# Change log

## [v0.7.2] - unreleased
* Expand supported rouge versions
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please insert a 'Changed' header and keep an empty line before? (Please see previous entries for how it should look.) Can you also please modify to say 'Change gemspec to expand...' and add your full name with GitHub ref in brackets at the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (I hope I did it right? 😅)

CHANGELOG.md Outdated
@@ -1,5 +1,8 @@
# Change log

## [v0.7.2] - unreleased
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove version and keep the 'unreleased' only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ashmckenzie ashmckenzie force-pushed the ashmckenzie/update-rouge-dependency branch from e6ae34f to 7ad7f4d Compare March 7, 2023 03:14
@piotrmurach piotrmurach merged commit 429d046 into piotrmurach:master Mar 12, 2023
@piotrmurach
Copy link
Owner

Thank you ❤️

@piotrmurach
Copy link
Owner

Released version 0.7.2.

@ashmckenzie
Copy link
Contributor Author

Thanks @piotrmurach! 💜

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

@piotrmurach piotrmurach piotrmurach approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ashmckenzie @piotrmurach @bogdanvlviv