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

[Ruby]: Ruby / Messages bump #165

Merged
merged 16 commits into from
Sep 12, 2023
Merged

[Ruby]: Ruby / Messages bump #165

merged 16 commits into from
Sep 12, 2023

Conversation

luke-hill
Copy link
Contributor

🤔 What's changed?

Bumped minimum messages / ruby ver

⚡️ What's your motivation?

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, documentation etc. without changing behaviour)
  • 🐛 Bug fix (non-breaking change which fixes a defect)
  • ⚡ New feature (non-breaking change which adds new behaviour)
  • 💥 Breaking change (incompatible changes to the API)

♻️ Anything particular you want feedback on?

📋 Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

@@ -20,7 +20,7 @@ Gem::Specification.new do |s|
'source_code_uri' => 'https://github.com/cucumber/gherkin/blob/main/ruby'
}

s.add_runtime_dependency 'cucumber-messages', '>= 19.1.4', '< 22.1'
s.add_runtime_dependency 'cucumber-messages', '~> 22.0'
Copy link
Contributor

@mpkorstanje mpkorstanje Sep 7, 2023

Choose a reason for hiding this comment

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

As nothing materially changed about the messages for Gherkin, is there a reason not to increase the upper bound instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As in permit 19.1 but increase the upper limit to say 23?

Currently the maximum version is 22.0 so any revision in the 22.x should be fine here. Just trying to reduce the support surface area. In theory we could keep this support area the same?

Copy link
Contributor

@mpkorstanje mpkorstanje Sep 7, 2023

Choose a reason for hiding this comment

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

Yes. Allowing a wider range allows us to make major releases of messages without also making major releases of Gherkin.

This reduces the churn somewhat. Wouldn't worry about supported version ranges here. If something Gherkin related gets added to messages we'll have to use it in all Gherkin implementations (because of the single test suite).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean in theory nothing stops us from setting the upper limit to 30 by the same logic. Or having no upper limit.

Bundler flags things without an upper limit hence the twiddle key. In theory something could come in v23 which breaks things hence setting this value?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I would think '>= 19.1.4', '< 23' should be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make that change now.

@luke-hill
Copy link
Contributor Author

@mpkorstanje - Ideally we would add in another breaking change for ruby alongside this, to kill 2 birds with one stone before releasing. If you're happy to hold off a short amount of time

@luke-hill
Copy link
Contributor Author

In other news @mpkorstanje this is one that's slipped through. As this should trigger all ruby tests. I'll have a look to see the changes and see if I can work it out

CHANGELOG.md Outdated Show resolved Hide resolved
@mpkorstanje
Copy link
Contributor

mpkorstanje commented Sep 9, 2023

@luke-hill I've pushed some changes to minimize the diff. Also fixed the ruby tests no running for PR's on main. Thanks!

@luke-hill luke-hill merged commit 9739c0d into main Sep 12, 2023
7 checks passed
@luke-hill luke-hill deleted the refactor/minimum_ruby_bump branch September 12, 2023 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants