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

Allow keyword arguments to match an expectation expecting *only* positional arguments #732

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

floehopper
Copy link
Member

TODO:

  • Address to-do list in WIP commit
  • Check the examples below

This is a (very belated) possible fix for #593.

According to these docs:

Note that Ruby 3.0 doesn’t behave differently when calling a method which doesn’t accept keyword arguments with keyword arguments. For instance, the following case is not going to be deprecated and will keep working in Ruby 3.0. The keyword arguments are still treated as a positional Hash argument.

def foo(kwargs = {})
 kwargs
end

foo(k: 1) #=> {:k=>1}

This is because this style is used very frequently, and there is no ambiguity in how the argument should be treated. Prohibiting this conversion would result in additional incompatibility for little benefit.

This solution takes an alternative approach to #605 which depended on #532. Instead of basing the logic in Mocha::ParameterMatchers::PositionalOrKeywordHash#matches? on the original method signature, it bases it on the expected arguments, i.e. the parameter matchers. Since keyword arguments can only appear after all positional arguments, we can detect an expectation which only expects positional arguments by checking whether the last parameter matcher is expecting a positional Hash or a keyword Hash. If it's expecting a positional Hash then we can assume the expectation only expects positional arguments and thus it should successfully match keyword arguments with that positional Hash without issuing a deprecation warning.

I think my work on #532 at the time must've blinded me to the idea that we needed to base the logic on the original method signature which is a considerably more complicated undertaking. Although the solution in this PR might not be perfect, I think it's probably an improvement and we can look at ensuring that expected parameters match the original method signature as a separate issue which relates to #149, #531 & #532.

Before fix

foo = mock('foo')
foo.stubs(:bar).with(123, { baz: 456 })

Mocha::Configuration.override(strict_keyword_argument_matching: false) do
  foo.bar(123, { baz: 456 }) # => matched *without* deprecation warning
  foo.bar(123, baz: 456) # => matched *with* deprecation warning
end

Mocha::Configuration.override(strict_keyword_argument_matching: true) do
  foo.bar(123, { baz: 456 }) # => matched *without* deprecation warning
  foo.bar(123, baz: 456) # => did not match
end

After fix

foo = mock('foo')
foo.stubs(:bar).with(123, { baz: 456 })

Mocha::Configuration.override(strict_keyword_argument_matching: false) do
  foo.bar(123, { baz: 456 }) # => matches *without* deprecation warning
  foo.bar(123, baz: 456) # => matches *without* deprecation warning
end

Mocha::Configuration.override(strict_keyword_argument_matching: true) do
  foo.bar(123, { baz: 456 }) # => matches *without* deprecation warning
  foo.bar(123, baz: 456) # => matches *without* deprecation warning
end

To `expected_value` to make the code clearer.
To `actual_values` to make the code clearer.
To `actual_value` and `is_last_parameter` to `is_last_actual_value` and
`#extract_parameter` to `#extract_actual_value` to make the code
clearer.
The test passing is more important than whether there was a deprecation
warning.

Also it means we can use a guard condition and thus avoid the
indentation in the `if` condition branch which I think makes the tests
more readable.
From `StrictKeywordArgumentMatchingTest`. The fact that the test class
name includes the words "strict keyword argument matching" makes these
suffixes redundant.

These should probably have been removed in this commit [1] in #730.

[1]: eb84bcd
TODO:
* Try to simpify logic in `PositionalOrKeywordHash#matches?`
* Maybe use keyword arguments for `PositionalOrKeywordHash` constructor
* Check whether all relevant scenarios are covered by tests
* Check whether this has any effect on #594 (I don't think it does)

* Fixes #593
* Supersedes #605
Previously, *all* of the tests in `StrictPositionalOrKeywordHashTest`
had a `if Mocha::RUBY_V27_PLUS` condition around them. Now the whole
test case has that condition which means we can simplify the `#setup`
and `#teardown` methods.

c.f. this commit [1].

[1]: 097446d
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.

1 participant