Skip to content
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

Relax rule: Allow mixing basic operators without parens (+-*/) #816

Closed
feross opened this Issue Mar 7, 2017 · 8 comments

Comments

6 participants
@feross
Copy link
Member

commented Mar 7, 2017

There is talk that the preventing the mixing of basic arithmetic operators like 4 + 5 * 2 is too restrictive. This was released in standard v9. Currently standard requires that 4 + 5 * 2 be written as 4 + (5 * 2).

There was discussion about this in the original issue here: #566 (comment) and it came up on Twitter here: https://twitter.com/wa7son/status/838310839560060928

I think this is acceptable. I personally care more about parens for the other precedence levels which are much less well-understood.

Also, note: When adding a new rule to standard that we all agree is worth having, I usually prefer to keep all the options on (so it's as restrictive as is reasonable), and then to relax the rule in a patch version as folks bring up issues like this one.

So this is actually feedback that I was expecting 😉 Please share thoughts below.

@feross feross added the enhancement label Mar 7, 2017

@dougwilson

This comment has been minimized.

Copy link

commented Mar 7, 2017

FWIW, I was meh to -1 on the rule when it was discussed, but not enough to debate it. I ran into one project so far where I had to make a change because of the rule, which was to make 'string' + 42 * 42 be 'string' + (42 * 42), which was imo an improvement for everyone. If anything, I think a rule that requires maths to be separated from the string concat + is a nice rule.

@utanapishtim

This comment has been minimized.

Copy link

commented Mar 7, 2017

I think the rule should be kept as is. While I appreciate concerns surrounding the restrictiveness of the rule, I think standard shines at making code explicit, and clear. Signifying precedence with parens falls right into this philosophy. Let us not forsake beginners nor forget the experience of encountering a new code base, sometimes two extra chars go a long way. imho.

@watson

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

I'm strongly in favour of relaxing this rule. I think it's the wrong strategy to drag everyone down to the average.

If we want to force everyone to write safe code, we should also ban --/++, force people to declare all variables in the top of the scope, not use Promises etc etc. There's no end to this!

We are in fact heading fast towards just recreating jslint. We have to ask our selfs if standard should be a style guide or if it should also be an error prevention tool. If it should also be an error prevention tool, we have to ask our selfs how experienced the developer is expected to be. We have have to be pragmatic about this. Do we expect developers to know entry-level algebra? Do we expect them to know how to use Promises? Is prototypal inheritance too complex for people to understand? Many people don't understand regular expressions, so maybe we should disallow those?

@Flet

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

When working with a group of developers on a single project, I've found that clarity in code is more productive then cleverness.

Adding parenthesis to signify precedence gives clarity to a non-trivial line of code, even if folks could comprehend it without them. I've rarely been upset at having extra parens as they usually help me break down the logic faster.

All that being said, I can see how this rule with all options on could get annoying pretty quick. No one likes feedback that their simple Arithmetic 2 * 10 + 1 is wrong because it doesn't have parenthesis.

Getting things back on track, here is the rule:
http://eslint.org/docs/rules/no-mixed-operators

And here are the default options:

{
    "no-mixed-operators": [
        "error",
        {
            "groups": [ // Sets that should require parenthesis for precedence differences
                ["+", "-", "*", "/", "%", "**"], // Arithmetic Operators
                ["&", "|", "^", "~", "<<", ">>", ">>>"], // Bitwise Operators
                ["==", "!=", "===", "!==", ">", ">=", "<", "<="], // Comparison Operators
                ["&&", "||"], // Logical Operators
                ["in", "instanceof"] // Relational Operators
            ],
            "allowSamePrecedence": true // ignore for operators that have same precedence
        }
    ]
}

Its unfortunate that there is no option for minimum number of operations on a line before checking, as I think that would be a great way to temper this rule.

I'm in favor of removing the Arithmetic operators from the rule as it would probably eliminate most of the annoyance.

@Flet

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

If anything, I think a rule that requires maths to be separated from the string concat + is a nice rule.

I agree with this @dougwilson unfortunately there is not a rule/option that does this yet :(

Is keeping the Arithmetic operators in this rule worth it for catching these cases?

@feross

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2017

@watson It's clear that you're really frustrated. Sorry if this rule change caused you trouble.

I agree that this rule should be relaxed, since we don't want standard to become too noisy by e.g. warning about common code that most people think is perfectly fine.

In the future, it would be great to hear your feedback in the "release proposal" issue I always open before putting out a new major release. Here's the one for 9.0.0. It would be really helpful to get feedback then so we can catch issues like this before they go out in a new release.

Speaking of which, here is the release proposal for 10.0.0 that would be great to get your feedback on. This release adds two big changes: warn about deprecated Node APIs (so that maybe we can convince Node core this is a better way to handle deprecations than adding console warnings!) and enforce node.js-style callbacks get called with an error or null as the first argument. Please share feedback.

feross added a commit to standard/eslint-config-standard that referenced this issue Mar 7, 2017

@feross

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2017

The rule has been relaxed in standard 9.0.1 and 10.0.0-beta.1. Cheers 🍻

@dcousens

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

"**"

Does anyone have a reference I can find as to when this was added to JS? TIL

edit: found https://blog.mariusschulz.com/2015/11/24/the-exponentiation-operator-in-javascript

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.