[Fix #7849] Add new Lint/AmbiguousOperatorPrecedence cop
#10047
Conversation
dvandersluis
changed the title
Lint/AmbiguousOperatorPrecedence cop
|
TBH, it looks redundant style to me as many changes mean. I think this cop should be disabled by default. |
|
Also, I think this is a style dept, not a lint dept, because users may have different tastes. |
| # # good (same precedence) | ||
| # a + b + c | ||
| # a * b / c % d | ||
| class AmbiguousOperatorPrecedence < Base |
There was a problem hiding this comment.
Ambiguous looked weird because operator precedence is clear in the Ruby specification. How about name like Style/OperatorPrecedenceParentheses?
There was a problem hiding this comment.
I think Ambiguous makes sense because it's aligned with the other Ambiguous cops -- flagging syntax that, while clearly defined for ruby itself, is not necessarily clear for a user reading it, especially one who is not well-versed in Ruby.
However, if it's not desired here, ExplicitOperatorPrecedence could be a better name.
There was a problem hiding this comment.
ExplicitOperatorPrecedencecould be a better name.
I also first came up with the same name :-) However, I've withheld the name because I'm a little confused as to whether it's Explicit or Implicit. But I think it's a better name than Ambiguous.
There was a problem hiding this comment.
Yeah, probably we can do a bit better here, although the point is more that the precedence might be ambiguous for the people, not for Ruby. :-) It's clear that as far as Ruby's concerned there's no ambiguity, yet still people make a ton of mistakes with logical expressions, based on wrong assumptions about the priorities there. A more neutral name can be just LogicalOperatorPrecedence with styles like "Explicit" and "Implicit" if we want to appease everyone.
There was a problem hiding this comment.
I would be strongly against making it configurable in a way that makes these parens an offense, and otherwise "Implicit" style would probably just be choosing to disable the cop 😅
|
If it's desired to move this to I am ok with making this disabled by default as well, but I think it should be at the minimum enabled for rubocop itself, given @bbatsov's request for this in the first place. I'll defer to him though. |
I think .rubocop.yml of the repo should also be disabled (following the default). IMHO, it's tough for me that |
|
That's a valid point, although I have to say that the cop does exactly what I had in mind (therefore I like it) and I think that it should probably be enabled by default. Of course, I'm the type of person who's not afraid of extra parentheses. :D (and values clarity over brevity) I'm on the fence as to whether that's a Style or a Lint cop - it can certainly prevent very real mistakes in the code. I would definitely like to have it enabled at least for RuboCop's codebase, but I don't see the harm in leaving it to the users to decide if they should enable it (meaning giving it the usual "pending" status). |
|
Yeah, I fully agree that the implementation behavior is pretty to the expected behavior. However, I think it depends on the context whether it is preferable to have parentheses, so it is hard to be forced to do so uniformly. I sometimes hear "Default rules are too strict, but customization is hard...". I feel that this rule will increase it... |
Respectfully, I don't think this should matter, it's easy enough to look at the previous blame for a line, and we do plenty of this kind of correction that overwrites a wider blame historically. I don't think it should prevent us from making changes that affect our own style.
Definitely get this. I think this is a beneficial change and would likely enable it on my codebase if it's disabled by default, but I will leave it to you guys. My counterpoint would be that it's very hard to discover cops that exist but aren't enabled... |
That's why I like launching everything as pending and leaving it to the users to decide what they like. If we get very bad feedback about something we won't be enabling it by default in the next version. |
|
Let's go ahead with this cop and see how it goes. We can still change the name before the next release if someone comes up with a better name. |
As per @bbatsov's request a year ago (#7849), adds a cop that requires parentheses around operations within an expression with multiple operations of different precedence. It does not consider comparison or unary operations.
Please take a look at the second commit to see how rubocop itself is changed by this cop (it looks pretty good to me but if anything stands out we can customize).
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.