Fix double negative conditional bug in NegatedIf and NegatedWhile style rules
#3146
Conversation
|
cc @ivantsepp |
natalzia-paperless
changed the title
NegatedIf and NegatedWhile style rules
natalzia-paperless
force-pushed
the
fix_double_negative_conditional_bug
branch
3 times, most recently
from
880752b
to
089b385
Compare
| ['if !!condition', | ||
| ' some_method', | ||
| 'end']) | ||
| expect(cop.offenses).to be_empty |
There was a problem hiding this comment.
I think there should also be a test for modifier if usage (single-line if). Refer this case.
There was a problem hiding this comment.
Updated!
natalzia-paperless
force-pushed
the
fix_double_negative_conditional_bug
branch
from
089b385
to
c4409a8
Compare
|
Please squash the commits @natalzia-paperless |
natalzia-paperless
force-pushed
the
fix_double_negative_conditional_bug
branch
2 times, most recently
from
9d876cd
to
35e4ffc
Compare
|
@tejasbubane squashed! |
|
|
||
| _object, method = *condition | ||
|
|
||
| return unless method == :! && !(node.loc.respond_to?(:else) && |
There was a problem hiding this comment.
Seems to method == :! should be removed now.
There was a problem hiding this comment.
Hmmm I think we still need this check to see if the only operator is !. the additions here only rule out a double negation, which wouldn't be enough to check for this rule.
There was a problem hiding this comment.
Ah, yeah. I got confused here. Still, it seems to me we can directly check that we're dealing with single instead of double negation and that would simplify the logic. Right now the structure of the methods seems a bit convoluted.
|
I've added a couple of remarks. |
natalzia-paperless
force-pushed
the
fix_double_negative_conditional_bug
branch
2 times, most recently
from
934a2a9
to
7545baf
Compare
|
@bbatsov pushed some new code. how do you feel about this approach? it removed the |
natalzia-paperless
force-pushed
the
fix_double_negative_conditional_bug
branch
from
7545baf
to
3478ba7
Compare
Previously this file only looked at the first operator in the line With these changes there is now a pattern match to ensure `!!` isn't flagged by these rules Add tests for double negation in while and if specs Update CHANGELOG.md
natalzia-paperless
force-pushed
the
fix_double_negative_conditional_bug
branch
from
3478ba7
to
a4eca96
Compare
|
Yep, that's much better. |
…rubocop#3146) Previously this file only looked at the first operator in the line. With these changes there is now a pattern match to ensure `!!` isn't flagged by these rules.
Currently you receive a negated if/while error from Rubocop if you have a conditional that is double negated. Rubocop should have awareness of whether there is a single
!or two. In the event that your line reads something akin toif !!conditionalRubocop should only fail on the DoubleNegation rule.Before submitting the PR make sure the following are checked:
master(if not - rebase it)and description in grammatically correct, complete sentences.