-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
New rule: Disallow mixes of different operators (no-mixed-operators) #566
Comments
👍 Pretty sure this is taught as a common best practice anyway. I'm all for disallowing |
I agree with @sotojuan. Programmers who know what they're doing are probably already parenthesizing their statements. IMO it's naive to assume that someone writing low-level code is more likely to know what they're doing. Even with relatively simple operations parenthesizing helps a ton. // This is difficult to read, not complicated.
const foo = 3 + 4 / 5 + 3 + 5 * 4 + 6
// This is much easier to read.
const foo = 3 + (4 / 5) + 3 + (5 * 4) + 6 |
Agreed, if mixing: parenthesis. |
Yup, I still mess this up every now and then haha |
Targeting this for standard v9. |
This will be part of the standard v9 beta. We will check all operators except for bitwise operators, since it's very common in low-level modules to omit parens when dealing with binary operators. Maybe we can consider making this stricter in a future version, but I doubt we'll want to. The rule is already very aggressive.
Ecosystem effect is pretty large, but I looked at each failing example and agree that parens ought to be used in each instance to improve clarity. Many of the errors were in my own repos.
|
After fixing repos I control, ecosystem impact looks better, but still substantial.
|
I do use this pattern a bit: Edit: just read this more closely:
So IIUC the above pattern would not be affected. If so, LGTM Edit 2: lol you wrote "binary" and I read "boolean" but you meant "bitwise" |
Oh, sorry, I meant bitwise operators, like Personally, I find it confusing to read that line without the parens, even though I know I've written lines like that before. It's harder to visually parse what's going on when reading it, IMO |
Yeah ok, the parens aren't too bad. I note some irony since in the past I've seen it argued, possibly by me, that explicit semicolons everywhere is as superfluous as explicit parens everywhere. "Just learn these simple operator precedence rules, it's fine. Really!" I realise now that it's a false equivalence though, operator precedence has a great many more rules to remember than ASI. |
This is so true. 😄 Btw, this is just for the beta release, so worst case if it sucks we can revert before release. |
Not being able to use simple math with +, -, /, * without having to force parens seems annoying (i write a lot of math code). Bit-wise ops are confusing though in terms of preference! |
I can already hear my first grade math teacher yelling at me for not knowing the basic operation order |
@mafintosh We could turn it off for +, -, /, * since I agree that basic arithmetic order is well-understood. But on longer lines it's still helpful, e.g. return value - Math.floor((value - min) / range) * range vs. return value - (Math.floor((value - min) / range) * range) |
I don't personally find that helpful but i get your point. Stuff like this is incredible hard to parse though and i mess up ALL the time so I'd def be okay with enforcing it there
|
@feross A more common mistake I've seen is something like return value - a + b Where someone ment return value - (a + b) So just forcing them all over the place for the classic ops makes it harder to question if someone ment to add them or not imo |
@mafintosh The rule currently doesn't warn about operators that have the exact same precedence, so |
http://eslint.org/docs/rules/no-mixed-operators
What do folks think about this rule?
I've always found it super confusing when multiple operators with similar precedence are used without explicit parens. For example:
There are currently a lot of repos that fail when this rule is enabled:
Maybe we can limit it a bit to make it more palatable? There are lots of possible errors. These were pretty common:
The last two are basic algebra order-of-operations that folks usually learn in elementary/middle school, so maybe we can skip warning for those? And the warnings involving
<<
happen on modules that are super low-level where we might be able to assume the programmer knows what they're doing.If we just warn when '&&' and '||' are used together without explicit parens, then we get a more reasonable number of warnings:
Curious to get people's thoughts. Is this generally helpful, or just a problem that I have? 😉
The text was updated successfully, but these errors were encountered: