Update: Fix no-extra-parens false negative (fixes #7229)

[not-an-aardvark]

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

See #7229 for more info on the bug.

Please check each item to ensure your pull request is ready:

• I've read the pull request guide
• I've included tests for my change
• (n/a) I've updated documentation for my change (if appropriate)

What changes did you make? (Give an overview)

Previously, the nestedBinaryExpressions: false option would cause no-extra-parens to ignore all binary expressions, even non-nested ones.

/* eslint no-extra-parens: [2, "all", {"nestedBinaryExpressions": false}]*/

(foo) + baz // foo is not nested, so this should be reported
(foo * bar) + baz // (foo * bar) is itself a binary expression, so this should not be reported.

Is there anything you'd like reviewers to focus on?

[mention-bot]

@not-an-aardvark, thanks for your PR! By analyzing the annotation information on this pull request, we identified @vitorbal, @mysticatea and @kaicataldo to be potential reviewers

[eslintbot]

LGTM

[platinumazure]

LGTM, just want another set of eyes on this. (Also, since this isn't a regression from 3.6.0, we may want to hold off until we know if there will be a patch release.)

[kaicataldo]

Mind adding a test to ensure non-nested LogicalExpressions warn for extra parens with this config in the invalid tests? Otherwise, LGTM!

[eslintbot]

LGTM

[not-an-aardvark]

Added some more tests for LogicalExpressions.

[kaicataldo]

Sorry, one last thing I didn't notice the first time around: the commit message should start with "Fix" instead of "Update", since this is a bug fix. Other than that, LGTM. Thanks for contributing!

[platinumazure]

@kaicataldo This will introduce more warnings-- bugfixes with more warnings are Updates and semver-minor, I believe.

[kaicataldo]

Agreed that this is a semver-minor change, but didn't realize we actually label them differently. In that case, LGTM.

[kaicataldo]

Ah, I see. From http://eslint.org/docs/developer-guide/contributing/pull-requests#step-2-make-your-changesa-namestep2a

Update - for a backwards-compatible enhancement or a change to a rule that increases the number of reported problems.

Makes sense, just something I didn't know. TIL!

[platinumazure]

Might be an artifact of eslint-release using those tags to determine
whether a release should be major/minor/patch. Not 100% sure though.

On Sep 24, 2016 4:56 PM, "Kai Cataldo" notifications@github.com wrote:

Ah, I see. From http://eslint.org/docs/developer-guide/contributing/
pull-requests#step-2-make-your-changesa-namestep2a

Update - for a backwards-compatible enhancement or a change to a rule that
increases the number of reported problems.

Makes sense, just something I didn't know. TIL!

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#7231 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARWeo-vO1UG7sVowFOAxtG0bcO1vEgVks5qtZyhgaJpZM4KFvxC
.

[vitorbal]

@platinumazure you're correct! eslint-release uses those tags to determine which type of release it should be.

[kaicataldo]

LGTM. Thanks for contributing to ESLint!