Closed pirj closed 5 months ago
luke-hill
commented
5 months ago Whilst I'm not explicitly against this, we could just guard against this for the 1 instance where it auto-generates a pre-built parameter type called bigdecimal.
Looking at the gem requirement it's quite big. I don't mind if this is something you think is a better practice, rspec is clearly a big gem. But I think just guarding vs this isn't a huge amount of work and only affects something 0.01% of people use in ruby
pirj
commented
5 months ago Hi @luke-hill
A bit of background to make sure we’re talking about the same things.
require “bigdecimal” statement in the code https://github.com/cucumber/cucumber-expressions/blob/c07b31f05315c2a35ac13f2e0f5d5a36304f57bb/ruby/lib/cucumber/cucumber_expressions/parameter_type_registry.rb#L6It happens that RSpec is running their own spec suite against ruby-head (future Ruby 3.4). And RSpec has no runtime dependency on bigdecimal. But RSpec’s suite is using cucumber. And it fails when the bundler can’t find the bigdecimal gem.
Other projects that: 1) use cucumber 2) run their specs on ruby-head 3) don’t depend on bigdecimal would encounter the same issue. There are just not many of them i guess.
I’m uncertain if I understand the suggestion to add a guard against a pre-built parameter, as I have zero knowledge how this code works. I’ll gladly make a change if you guide me.
Should we just remove the require statement? This satisfies me as Cucumber user. Would it work for you as the maintainer? What would happen when such a parameter type is created?
luke-hill
commented
5 months ago So essentially the default argument is that pre ruby 3.3 it is, but now it isn't. So I'm mindful of this e.t.c. I'd prefer some ruby guard vs blanket.
We generate a couple of very small instances of bigdecimal references. So either guarding vs those or just removing those would be better. I'm happy to make change/s
If people want to make bigdecimal items, they would just need to ensure they have required it, so it's up to the user. The issue is we have about 10 or so default ones (One of which is this).
pirj
commented
5 months ago Up to you and no pressure - ruby-head is an optional CI check for us, just a sore in the eye.
luke-hill
commented
5 months ago I think what I'll likely do is do a bugfix / patch release for the quick fix which introduces this requirement and then maybe remove it properly in a major.
luke-hill
commented
5 months ago @pirj - If you add a changelog entry to this. I'll accept and merge this in and quickly release a bugfix release (To hopefully fix some issues your end).
We'll then remove the dependency in the next major
pirj
commented
5 months ago Done
aslakhellesoy
commented
5 months ago Hi @pirj,
Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾
In return for this generous offer we hope you will:
On behalf of the Cucumber core team, Aslak Hellesøy Creator of Cucumber
🤔 What's changed?
bigdecimalis now declared as a dependency⚡️ What's your motivation?
https://github.com/rspec/rspec-mocks/actions/runs/7659693178/job/20875374161?pr=1477
⚠️ https://github.com/cucumber/cucumber-expressions/pull/272 needs to be merged first? Or we should see a failure there in ruby-head.
🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
📋 Checklist:
This text was originally generated from a template, then edited by hand. You can modify the template here.