Skip to content
This repository has been archived by the owner on Nov 30, 2024. It is now read-only.

Allow string keys for method kwarg splats #591

Merged
merged 5 commits into from
Jan 22, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions lib/rspec/support/method_signature_verifier.rb
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ def invalid_kw_args_from(given_kw_args)
def has_kw_args_in?(args)
Hash === args.last &&
could_contain_kw_args?(args) &&
(args.last.empty? || args.last.keys.any? { |x| x.is_a?(Symbol) })
(args.last.empty? || args.last.keys.any? { |x| x.is_a?(Symbol) || x.is_a?(String) })
end

# Without considering what the last arg is, could it
Expand Down Expand Up @@ -364,14 +364,7 @@ def unlimited_args?

def split_args(*args)
Copy link
Contributor Author

@malcolmohare malcolmohare Jan 18, 2024

Choose a reason for hiding this comment

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

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

kw_args = if @signature.has_kw_args_in?(args)
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
args.pop.select { |k, _| k.is_a?(Symbol) || k.is_a?(String) }.keys
else
[]
end
Expand Down
66 changes: 60 additions & 6 deletions spec/rspec/support/method_signature_verifier_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -384,17 +384,21 @@ def arity_kw(x, y = {}, z:2); end
expect(valid?(nil, :a => 1)).to eq(false)
end

it 'treats symbols as keyword arguments and the rest as optional argument' do
expect(valid?(nil, 'a' => 1)).to eq(true)
expect(valid?(nil, 'a' => 1, :z => 3)).to eq(true)
expect(valid?(nil, 'a' => 1, :b => 3)).to eq(false)
expect(valid?(nil, 'a' => 1, :b => 2, :z => 3)).to eq(false)
it 'treats symbols as keyword arguments' do
expect(valid?(nil, :z => 3)).to eq(true)
expect(valid?(nil, :b => 3)).to eq(false)
expect(valid?(nil, :b => 2, :z => 3)).to eq(false)
end

it 'treats string keys as invalid keyword arguments' do
expect(valid?(nil, 'a' => 1)).to eq(false)
expect(valid?(nil, 'a' => 1, :z => 3)).to eq(false)
end
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


it 'mentions the invalid keyword args in the error', :pending => RSpec::Support::Ruby.jruby? && !RSpec::Support::Ruby.jruby_9000? do
expect(error_for(1, 2, :a => 0)).to eq("Invalid keyword arguments provided: a")
expect(error_for(1, :a => 0)).to eq("Invalid keyword arguments provided: a")
expect(error_for(1, 'a' => 0, :b => 0)).to eq("Invalid keyword arguments provided: b")
expect(error_for(1, 'a' => 0, :b => 0)).to eq("Invalid keyword arguments provided: a, b")
malcolmohare marked this conversation as resolved.
Show resolved Hide resolved
end

it 'describes invalid arity precisely' do
Expand Down Expand Up @@ -656,6 +660,53 @@ def arity_required_kw_splat(w, *x, y:, z:, a: 'default'); end
end
end

describe 'a method with only a keyword arg splat' do
eval <<-RUBY
def arity_kw_arg_splat(**rest); end
RUBY

let(:test_method) { method(:arity_kw_arg_splat) }

it 'allows undeclared keyword args' do
expect(valid?(:x => 1)).to eq(true)
expect(valid?(:x => 1, 'y' => 2)).to eq(true)
expect(valid?('y' => 2)).to eq(true)
end

it 'does not allow kw args of only unsupported types' do
expect(valid?(3 => 1)).to eq(false)
expect(valid?({"a":"b"} => 1)).to eq(false)
end

it "ignores kwargs with invalid types if there are any with valid types" do
expect(valid?(:x => 1, 3 => 10)).to eq(true)
expect(valid?(:x => 1, 'y' => 2, 3 => 10)).to eq(true)
expect(valid?('y' => 2, 3 => 10)).to eq(true)
end

it 'mentions the required kw args and keyword splat in the description' do
expect(signature_description).to \
eq("any additional keyword args")
end

describe 'with an expectation object' do
it 'allows zero non-kw args' do
expect(validate_expectation 0).to eq(true)
expect(validate_expectation 1).to eq(false)
expect(validate_expectation 0, 0).to eq(true)
expect(validate_expectation 0, 1).to eq(false)
end

it 'does not match unlimited arguments' do
expect(validate_expectation :unlimited_args).to eq(false)
end

it 'matches arbitrary keywords' do
expect(validate_expectation :arbitrary_kw_args).to eq(true)
end
end
end

describe 'a method with required keyword arguments and a keyword arg splat' do
eval <<-RUBY
def arity_kw_arg_splat(x:, **rest); end
Expand All @@ -666,6 +717,7 @@ def arity_kw_arg_splat(x:, **rest); end
it 'allows extra undeclared keyword args' do
expect(valid?(:x => 1)).to eq(true)
expect(valid?(:x => 1, :y => 2)).to eq(true)
expect(valid?(:x => 1, :y => 2, 'z' => 3)).to eq(true)
end

it 'mentions missing required keyword args in the error' do
Expand Down Expand Up @@ -730,7 +782,9 @@ def arity_kw_arg_splat(x, **rest); end
it 'allows a single arg and any number of keyword args' do
expect(valid?(nil)).to eq(true)
expect(valid?(nil, :x => 1)).to eq(true)
expect(valid?(nil, 'x' => 1)).to eq(true)
expect(valid?(nil, :x => 1, :y => 2)).to eq(true)
expect(valid?(nil, :x => 1, :y => 2, 'z' => 3)).to eq(true)
expect(valid?(:x => 1)).to eq(true)
expect(valid?(nil, {})).to eq(true)

Expand Down
Loading