[Fix #5388] Add new Style/UnlessLogicalOperators cop.
#9386
Conversation
caalberts
force-pushed
the
cop-style-unless-mixed-conditions
branch
2 times, most recently
from
316a479
to
0114d5f
Compare
|
It seems to me it'd better if this cop had some degree of configuration - e.g. it could enforce only simple conditions (no logical operators) or allow something like the currently suggested default. |
|
@bbatsov I agree it would be better to have configurable styles. I just wasn't sure what are the options to go with, so I started with this one first. Do you have suggestions on what to name the options? |
caalberts
force-pushed
the
cop-style-unless-mixed-conditions
branch
from
0114d5f
to
3cb2524
Compare
|
@bbatsov I added the 2 configurations suggested. What do you think? |
caalberts
changed the title
Style/UnlessMixedConditions cop.Style/UnlessLogicalOperators cop.
caalberts
changed the title
Style/UnlessLogicalOperators cop.Style/UnlessLogicalOperators cop.
|
I don't see any tests/mentions of |
|
@caalberts ping :-) |
@bbatsov that's a great catch! Thanks for highlighting. I'll update the PR with the necessary changes. |
caalberts
force-pushed
the
cop-style-unless-mixed-conditions
branch
2 times, most recently
from
c05184a
to
fcc893a
Compare
| end | ||
|
|
||
| def mixed_precedence_and?(node) | ||
| node.source.include?('&&') && node.source.include?('and') |
There was a problem hiding this comment.
I'm not sure if there is a better way to distinguish a && b && c and a && b and c.
Using node matcher doesn't work because both have the following AST:
(and
(and
(send nil :a)
(send nil :b))
(send nil :c))
Same goes for a || b || c and a || b or c.
There was a problem hiding this comment.
I considered using a node matcher for a and b && c, using (if (and ... (and ...)) ...) to match the following AST:
(and
(send nil :a)
(and
(send nil :b)
(send nil :c)))
However, if we need to check the node.source anyway, then it's unnecessary to have the node matcher.
There was a problem hiding this comment.
That's ok. Technically you don't need to use include?, though as you can do an equality check instead (and I guess it'd be faster).
There was a problem hiding this comment.
Could you share an example how we can do an equality check on this?
There was a problem hiding this comment.
Isn't this just a regular string? How about ==? :-)
|
@bbatsov Could you take a look again please? |
config/default.yml
Outdated
| VersionAdded: '<<next>>' | ||
| EnforcedStyle: forbid_mixed_logical_operator | ||
| SupportedStyles: | ||
| - forbid_mixed_logical_operator |
There was a problem hiding this comment.
Please, make those plural (operators).
| module RuboCop | ||
| module Cop | ||
| module Style | ||
| # This cop checks for the use of logical operators in an unless condition. |
There was a problem hiding this comment.
You can add a bit of rationale here - e.g. this cop exists to discourage hard to read/process usages of unless.
There was a problem hiding this comment.
In the rationale you should also briefly explain the purpose of the two supported styles.
| class UnlessLogicalOperators < Base | ||
| include ConfigurableEnforcedStyle | ||
|
|
||
| FORBID_MIXED_LOGICAL_OPERATOR = 'Do not use mixed logical operators in an unless condition.' |
There was a problem hiding this comment.
Put unless in backticks please.
| include ConfigurableEnforcedStyle | ||
|
|
||
| FORBID_MIXED_LOGICAL_OPERATOR = 'Do not use mixed logical operators in an unless condition.' | ||
| FORBID_LOGICAL_OPERATOR = 'Do not use a logical operator in an unless condition.' |
|
Just added a couple more small remarks. |
caalberts
force-pushed
the
cop-style-unless-mixed-conditions
branch
from
fcc893a
to
b14d839
Compare
caalberts
force-pushed
the
cop-style-unless-mixed-conditions
branch
from
b14d839
to
24bde17
Compare
[Fix #5388] Add new
Style/UnlessLogicalOperatorscop.This cop checks for the use of
&&and||in an unless condition.Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.mdif the new code introduces user-observable changes. See changelog entry format for details.