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

Ensure that has_<n> matcher works silently with keyword arguments #1187

Merged

Conversation

JonRowe
Copy link
Member

@JonRowe JonRowe commented May 26, 2020

Alternative to #1184 that doesn't use rspec-support meta magic.

@JonRowe JonRowe force-pushed the remove-kw-args-warning-from-has-matcher-without-rspec-support branch 2 times, most recently from 880bf3c to 7a12f66 Compare May 26, 2020 14:14
@JonRowe JonRowe force-pushed the remove-kw-args-warning-from-has-matcher-without-rspec-support branch from 7a12f66 to 23c1de7 Compare May 26, 2020 15:41
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

👍 Nice!

@JonRowe
Copy link
Member Author

JonRowe commented May 31, 2020

@pirj @benoittgt how do we feel about this? Realistically the only repetition we could avoid with this approach is the final send with / without keywords, it means a lot more string evals but should be as performant as the original which I know my support branch is not.

Whats interesting is that *args, **kwargs is not the equivalent of *args, as an empty hash is sent if you are not expecting keyword args... hence the if check surrounding empty kwargs.

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

On 2.7.1 I'm getting this:

  1) expect(...).to be_predicate handles keyword arguments to the predicate
     Failure/Error: raise "Warnings were generated: #{output}" if has_output?

     RuntimeError:
       Warnings were generated: /Users/pirj/source/rspec-dev/repos/rspec-core/lib/rspec/core/example_group.rb:764: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
       /Users/pirj/source/rspec-dev/repos/rspec-expectations/lib/rspec/matchers.rb:959: warning: The called method `method_missing' is defined here
...
rspec ./spec/rspec/matchers/built_in/be_spec.rb:84 # expect(...).to be_predicate handles keyword arguments to the predicate

Comment on lines +254 to +256
if @kwargs.empty?
@predicate_matches = actual.__send__(method_name, *@args, &@block)
else
Copy link
Member

Choose a reason for hiding this comment

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

What fails if we remove this?

k={}
def a(x)
end
def b(y, z: 1)
end
a(1, **k)
b(2, **k)

neither of those calls results in a warning 😕

Copy link
Member

Choose a reason for hiding this comment

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

I see the failures, but I'm having hard times understanding it, especially that the ArgumentError isn't raised where it should.

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't cause a warning, it sends an extra parameter to some places not expecting it.

Copy link
Member Author

Choose a reason for hiding this comment

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

e.g. splatting keyword arguments, creates keyword arguments, and nil is not {}

Copy link
Member

Choose a reason for hiding this comment

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

you need the master branch of rspec-core to avoid that warning

👍 that recent change slipped my mind.

Got it. So I've played around with it, and it seems that be_predicate() is sending __send__(:predicate, *[], **{}) and **{} goes for a regular parameter as {} 🤦

def m(p)
  puts p
end

__send__(:m, *[], **{})
warning: Passing the keyword argument as the last hash parameter is deprecated
{}

I don't have enough words to express my grief.

@JonRowe
Copy link
Member Author

JonRowe commented Jun 1, 2020

@pirj you need the master branch of rspec-core to avoid that warning

Copy link
Member

@benoittgt benoittgt left a comment

Choose a reason for hiding this comment

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

Lot's of binding.eval but the result is clear. LGTM!

@JonRowe JonRowe merged commit c58c7c4 into master Jun 1, 2020
@JonRowe JonRowe deleted the remove-kw-args-warning-from-has-matcher-without-rspec-support branch June 1, 2020 14:56
JonRowe added a commit that referenced this pull request Jun 1, 2020
JonRowe added a commit that referenced this pull request Jun 14, 2020
…atcher-without-rspec-support

Ensure that has_<n> matcher works silently with keyword arguments
JonRowe added a commit that referenced this pull request Jun 14, 2020
yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 6, 2021
…rspec/remove-kw-args-warning-from-has-matcher-without-rspec-support

Ensure that has_<n> matcher works silently with keyword arguments

---
This commit was imported from rspec/rspec-expectations@9a2f90c.
yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 6, 2021
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