cucumber / cucumber-ruby

Cucumber for Ruby. It's amazing!
https://cucumber.io
MIT License
5.18k stars 1.12k forks source link

Remove unknown hash keyword arguments #1757

Closed JonRowe closed 2 months ago

JonRowe commented 4 months ago

Description

Ruby head adds a capacity keyword option to Hash which causes other keywords passed to Hash.new to be validated, this causes an error when running Cucumber 9.2.0 on ruby-head (example from an rspec-expectations build):

/home/runner/work/rspec-expectations/rspec-expectations/bin/cucumber
<internal:hash>:37:in 'initialize': unknown keywords: :strict, :proc (ArgumentError)
    from /home/runner/work/rspec-expectations/rspec-expectations/bundle/ruby/3.4.0+0/gems/cucumber-9.2.0/lib/cucumber/multiline_argument/data_table.rb:78:in 'Class#new'

Type of change

Please delete options that are not relevant.

Checklist:

luke-hill commented 4 months ago

Do you know of any locations where more "easy to understand" discussion is ongoing.

I had a look at the initial PR's but I feel (Albeit late at night), I don't really "get it" other than it is more optimal for large hashes.

The code in question is very old and I'd rather not edit it without knowing exactly what the previous behaviour was doing.

JonRowe commented 4 months ago

So the commit that causes the issue is: https://github.com/ruby/ruby/commit/9594db0cf28d7bc10bfc46142239191a11f1dbbe you can see from the tests that unexpected keywords are expected to now raise an error.

I went sperlunking through the code and the original definition was actually passing a hash into Hash.new which would make that a default value for the hash, later formatting changes dropped the hash brackets which I think means that when Ruby 3.4.0 starts accepting keyword arguments the behaviour changes to this failure, but my original fix is wrong, whats actually required is to restore the brackets.

JonRowe commented 4 months ago

As an aside, rubocop also seems to fail on main? But this patch now makes tests pass locally

luke-hill commented 4 months ago

rubocop failing is a sep issue. Upgrading to 1.61 fixes it which I have done elsewhere. So don't worry about CI for now. I'll take a re-read of the stuff you sent over. In principle I have no issues just this is super ancient code.

JonRowe commented 4 months ago

Rebased and checked locally that rubocop passes, and tests pass on Ruby 3.3, on 3.4 there is also the need to bring in the base64 gem, and some output stuff has changed but I figure those changes are out of scope for this PR? Although I could easily add the dependency

luke-hill commented 3 months ago

I've enqueued the pipelines for now. I figure this does some improvements and I'm working on a v10 release to tie in with getting a lot of the dependencies updated and interopping again. So I will come back to this when I can

luke-hill commented 3 months ago

Forgot to post here - The failure in CI is unrelated and I'm working on fixing this up - it's something we should have done differently in the CCK.

@JonRowe - If you want to write up a changelog entry I'll get this merged ASAP - The next version cut will be v10 as we're doing some big changes to the core hexagon and updating the minimum ruby support

JonRowe commented 3 months ago

@luke-hill I've added a changelog entry and rebased

luke-hill commented 2 months ago

Thanks for this @JonRowe - All merged in now.

V10 is likely going to be a bit bigger as we move some of the internal requirements out into cucumber-core, so I'm not sure entirely when this will be released. But it'll be in the next version

JonRowe commented 2 months ago

Thanks!