-
-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add cop to enforce refute_includes over refute(collection.include?(actual)) #15
Add cop to enforce refute_includes over refute(collection.include?(actual)) #15
Conversation
1fb6b7b
to
25f116b
Compare
Not sure why CI fails. It passes on my local. I did run the documentation command before pushing. can anyone please help me with it? |
This repository allow ruby-head to fail. There is no problem if other matrices are successful. |
assert_offense(<<~RUBY, @cop) | ||
class FooTest < Minitest::Test | ||
def test_do_something | ||
refute(collection.includes?(actual)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check against include
method instead of includes
method.
rubocop/minitest-style-guide#17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@koic : Resolved the typo and squashed my commits. Please have a look.
d4f63e3
to
16ba08e
Compare
This PR has conflicts. Can you rebase with the latest master branch? |
16ba08e
to
a5f5f25
Compare
@koic : Resolved merge conflict. Please have a look. |
module RuboCop | ||
module Cop | ||
module Minitest | ||
# Check if your test uses `refute_includes` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is worded confusingly as now it implies that refutes_includes
is what's bad.
RUBY | ||
end | ||
|
||
def test_registers_offense_when_using_refute_with_include_and_variable_message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as with the other PR - I don't see a point in having separate tests for a variable message and a constant message. Relying on the param count should be enough here.
a5f5f25
to
0d0db24
Compare
@bbatsov : Resolved both the comments. Can you please have a look? |
RUBY | ||
end | ||
|
||
def test_registers_offense_when_using_refute_with_include_and_constant_message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still this pointless test with a constant message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. My bad. Removing it now.
# | ||
# @example | ||
# # bad | ||
# refute(collection.include?(actual)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use object
instead of actual
everywhere in the test.
…ctual)) trigger CI
0d0db24
to
9ab99d6
Compare
@bbatsov : Resolve the comments. |
Looks good. Thanks! |
PR adds cops to enforce the use of refute_includes over refute(collection.includes?(actual))
Ref: https://github.com/rubocop-hq/minitest-style-guide#refute-includes