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

Remove unknown hash keyword arguments #1757

Merged
merged 2 commits into from
Sep 10, 2024
Merged

Conversation

JonRowe
Copy link
Contributor

@JonRowe JonRowe commented Jul 14, 2024

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.

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • Tests have been added for any changes to behaviour of the code
  • New and existing tests are passing locally and on CI
  • bundle exec rubocop reports no offenses
  • RDoc comments have been updated
  • CHANGELOG.md has been updated

@luke-hill
Copy link
Contributor

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

JonRowe commented Jul 17, 2024

So the commit that causes the issue is: ruby/ruby@9594db0 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
Copy link
Contributor Author

JonRowe commented Jul 17, 2024

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

@luke-hill
Copy link
Contributor

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.

NULL_CONVERSIONS = Hash.new(strict: false, proc: ->(cell_value) { cell_value }).freeze
# This is a Hash being initialized with a default value of a Hash, DO NOT REFORMAT TO REMOVE {}
# Future versions [3.4.0+] of ruby will interpret these as keywords and break.
NULL_CONVERSIONS = Hash.new({strict: false, proc: ->(cell_value) { cell_value }}).freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you space out the arguments of the hash so { strict..... and rebase with main. Rubocop "should" now pass

@JonRowe
Copy link
Contributor Author

JonRowe commented Jul 18, 2024

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

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

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

JonRowe commented Aug 22, 2024

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

@luke-hill luke-hill merged commit a468bc6 into cucumber:main Sep 10, 2024
1 of 15 checks passed
@luke-hill
Copy link
Contributor

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 JonRowe deleted the patch-1 branch September 10, 2024 11:03
@JonRowe
Copy link
Contributor Author

JonRowe commented Sep 10, 2024

Thanks!

NULL_CONVERSIONS = Hash.new(strict: false, proc: ->(cell_value) { cell_value }).freeze
# This is a Hash being initialized with a default value of a Hash, DO NOT REFORMAT TO REMOVE {}
# Future versions [3.4.0+] of ruby will interpret these as keywords and break.
NULL_CONVERSIONS = Hash.new({ strict: false, proc: ->(cell_value) { cell_value } }).freeze
Copy link

Choose a reason for hiding this comment

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

Why not simply:

NULL_CONVERSIONS = { strict: false, proc: ->(cell_value) { cell_value } }.freeze

? The Hash.new seems redundant here.
Unless Hash here is not ::Hash?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll be honest, this is very old code and I don't know the precise way it is used. This was the smallest safest change suggested.

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.

3 participants