Allow string keys for method kwarg splats

[malcolmohare]

Attempting to fix: #554

The change expands the matching criteria for kwargs from x.is_a?(Symbol) to x.is_a?(Symbol) || x.is_a?(String). This now properly mirrors the behavior in the ruby docs.

https://docs.ruby-lang.org/en/3.0/syntax/calling_methods_rdoc.html#label-Hash+to+Keyword+Arguments+Conversion

Elsewhere tests were asserting that args with string keys were treated as optional arguments. I have not tested if this is true for ruby prior to ruby 3, but this is NOT true for ruby 3, and I've adjusted those tests.

Testing

bundle exec rspec spec/rspec/support/method_signature_verifier_spec.rb
...
Finished in 0.03491 seconds (files took 0.09141 seconds to load)
268 examples, 0 failures

Running script/run_build fails with a permissions error during rspec, so I don't have a clean test pass of the whole repo.

[pirj]

Question mark is missing.
This makes me think this branch is not covered in tests. Can you please check this?

[malcolmohare]

I think the whole chunk of code is unnecessary for the current ruby version. I went through the PR that added it, and the parameter declarations that PR was written to support no longer function (at least in my irb console which is ruby 3).

Example:

def foo(x, y = {}, z: 1) puts "#{y}, #{z}"; end
foo 1, 'a' => 2, 'b' => 2
ArgumentError (unknown keywords: "a", "b")

So i'm not sure what the correct thing to do here is. Do I update the code to respect ruby 3 functionality? Or does there need to be some if statement on the runtime version in the code?

[pirj]

We can conditionally define methods depending on the Ruby version/features.

Does the “any key type counts as an optional kwargs key” apply to all Ruby versions that suport optional kwargs?

If there’s a conditional branch that isn’t used as we’ve discovered, let’s double check why it was introduced initially. I’d remove if it’s dead code.

[malcolmohare]

This section was re-written so these comments are a bit outdated.

But in reference to what you are asking, yes the logic needs to stay for ruby < 3, and the proposed change is correct for ruby >= 3. The latest version of this PR introduces a RubyFeatures method to allow for this, and a link to the ruby changelog explaining the change.

[JonRowe]

This needs to be changed for Ruby 2.x

[JonRowe]

This needs to be changed to conditonally apply the new behaviour, the old spec was valid for Ruby 2.x

Typically we'd define whole different specs for different Rubies.

[malcolmohare]

Sure, added this as a RubyFeatures method and applied the change and the specs based on that flag.

[malcolmohare]

There's an alternative version of this function, let me know which is preferred.

def split_args(*args)
  return [args.length, []] unless @signature.has_kw_args_in?(args)
  kw_args = if RubyFeatures.kw_arg_separation?
              args.pop.keys
            else
              last = args.pop
              non_kw_args = last.reject { |k, _| k.is_a?(Symbol) }
              if non_kw_args.empty?
                last.keys
              else
                args << non_kw_args
                last.select { |k, _| k.is_a?(Symbol) }.keys
              end
            end
  [args.length, kw_args]
end

[JonRowe]

Thank you for this 👏

[JonRowe]

Released in 3.12.2 thank you!