Improve PredicateMatcher recommendations #2068
Conversation
This prepares for adding more mathcer options.
PredicateMatcher should not recommend changing `#key?` to `be_key`, but will now instead suggest using `have_key`.
| def to_predicate_method(matcher) | ||
| case matcher = matcher.to_s | ||
| when 'be_a', 'be_an', 'be_a_kind_of', 'a_kind_of', 'be_kind_of' |
There was a problem hiding this comment.
Subjectively, this reads better than multiple hash items
There was a problem hiding this comment.
I changed it to a hash lookup because one of the Metric cops said the method became too long. But I agree with you, and I will change it back when/if I get back to this PR.
| @@ -91,6 +91,8 @@ | |||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `be_something` matcher over `something?`. | |||
| expect(foo.has_key?('foo')).to be_truthy | |||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `have_key` matcher over `has_key?`. | |||
| expect(foo.key?('foo')).to be_truthy | |||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer using `have_key` matcher over `key?`. | |||
There was a problem hiding this comment.
Does this imply that the receiver is an instance of a Hash?
If it’s e.g. an SqlColumn, have_key? won’t fit, and it should be is_key? instead
There was a problem hiding this comment.
Can we make a distinction at least if there’s an argument?
There was a problem hiding this comment.
TIL key? is aliased as has_key? https://ruby-doc.org/3.4.1/Hash.html#method-i-include-3F
There was a problem hiding this comment.
Proposal:
Only auto-correct expect(foo.key?(foo)) with a single argument.
Only raise an offence with zero or multiple args or a block
There was a problem hiding this comment.
To be honest, I don't really like this cop (or others) that make broad assumptions about types which can not safely be deduced with static analysis. There will always be false positives.
There was a problem hiding this comment.
TIL key? is aliased as has_key?
And I thought one of them had been deprecated, but apparently that's not the case.
I also think this method is aliased as member? and include? btw.
| def to_predicate_matcher(name) | ||
| case name = name.to_s |
There was a problem hiding this comment.
Side note, unrelated to this PR. This list is incomplete, since RSpec has support for eg start_with/end_with. They work with Arrays, too. Also cover/exist/...
| if TO_PREDICATE_MATCHER_MAP.key?(name) | ||
| TO_PREDICATE_MATCHER_MAP.fetch(name) | ||
| elsif name.start_with?('has_') | ||
| name.to_s.sub('has_', 'have_')[0..-2] | ||
| else |
There was a problem hiding this comment.
How do you feel if we raise an offence, but don't autocorrect the "else" cases?
PredicateMatcher should not recommend changing
#key?tobe_key, but will now instead suggest usinghave_key.Fixes #2067
Before submitting the PR make sure the following are checked:
master(if not - rebase it).CHANGELOG.mdif the new code introduces user-observable changes.bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).