Skip to content

Explicitly declare bigdecimal as a dependency #273

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

Merged
merged 1 commit into from
Feb 20, 2024
Merged

Conversation

pirj
Copy link
Contributor

@pirj pirj commented Jan 25, 2024

🤔 What's changed?

bigdecimal is now declared as a dependency

⚡️ What's your motivation?

https://github.com/rspec/rspec-mocks/actions/runs/7659693178/job/20875374161?pr=1477

/home/runner/work/rspec-mocks/rspec-mocks/bundle/ruby/3.4.0+0/gems/cucumber-cucumber-expressions-17.0.1/lib/cucumber/cucumber_expressions/parameter_type_registry.rb:6:in `require': cannot load such file -- bigdecimal (LoadError)
	from /home/runner/work/rspec-mocks/rspec-mocks/bundle/ruby/3.4.0+0/gems/cucumber-cucumber-expressions-17.0.1/lib/cucumber/cucumber_expressions/parameter_type_registry.rb:6:in `<top (required)>'

⚠️ #272 needs to be merged first? Or we should see a failure there in ruby-head.

🏷️ What kind of change is this?

  • 🐛 Bug fix (non-breaking change which fixes a defect)

♻️ 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.

Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

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

Looks good but please also update the CHANGELOG with a ### Fixed entry. The format is explained in the document.

@mpkorstanje mpkorstanje self-assigned this Feb 15, 2024
@luke-hill
Copy link
Contributor

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
Copy link
Contributor Author

pirj commented Feb 16, 2024

Hi @luke-hill

A bit of background to make sure we’re talking about the same things.

  1. cucumber-expressions have a require “bigdecimal” statement in the code
  2. The bigdecimal is not a default gem in Ruby 3.4, and gems that need BigDecimal should specify the gem in their runtime dependencies

It 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
Copy link
Contributor

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
Copy link
Contributor Author

pirj commented Feb 16, 2024

Up to you and no pressure - ruby-head is an optional CI check for us, just a sore in the eye.

@luke-hill
Copy link
Contributor

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
Copy link
Contributor

@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

/home/runner/work/rspec-mocks/rspec-mocks/bundle/ruby/3.4.0+0/gems/cucumber-cucumber-expressions-17.0.1/lib/cucumber/cucumber_expressions/parameter_type_registry.rb:6:in `require': cannot load such file -- bigdecimal (LoadError)
	from /home/runner/work/rspec-mocks/rspec-mocks/bundle/ruby/3.4.0+0/gems/cucumber-cucumber-expressions-17.0.1/lib/cucumber/cucumber_expressions/parameter_type_registry.rb:6:in `<top (required)>'
@pirj
Copy link
Contributor Author

pirj commented Feb 20, 2024

Done

@luke-hill luke-hill merged commit db9ec46 into cucumber:main Feb 20, 2024
@aslakhellesoy
Copy link
Contributor

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:

  • ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
  • 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
  • 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
  • ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
  • 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

@pirj pirj deleted the patch-2 branch February 20, 2024 10:38
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.

4 participants