Update: Add support for parens on left side for-loops (fixes: #8393) #8679

VictorHom · 2017-06-02T02:48:54Z

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:

What changes did you make? (Give an overview)
Added a fix around supporting parens support for for-in and for-of loops which weren't giving off warnings. I updated no-extra-parens.js with supporting tests.

Is there anything you'd like reviewers to focus on?
I wrote a function requiresAfterSpace to determine if a space is required after a node. I didn't know a good way to test this and wasn't sure if what I have is optimal (not sure what I don't know). If you could provide some tips on what a good approach would be to fixing these issues, that would be great. I followed the docs from http://eslint.org/docs/developer-guide/working-with-rules and

eslintbot · 2017-06-02T02:48:55Z

LGTM

mention-bot · 2017-06-02T02:48:56Z

@VictorHom, thanks for your PR! By analyzing the history of the files in this pull request, we identified @michaelficarra, @not-an-aardvark and @vitorbal to be potential reviewers.

not-an-aardvark

Thanks! This looks good, I just have a small suggestion.

Also, can you change the prefix of the commit message to Update: rather than Fix:? Our tooling take Update: as an indication that a change is semver-minor according to our semantic versioning policy, and this commit is semver-minor because it's a bugfix that causes a rule to report more errors.

not-an-aardvark · 2017-06-03T07:17:11Z

lib/rules/no-extra-parens.js

+
+            return rightParenToken && tokenAfterRightParen &&
+                !sourceCode.isSpaceBetweenTokens(rightParenToken, tokenAfterRightParen) &&
+                tokenAfterRightParen.type === "Keyword";


Instead of checking whether the rightmost token is a keyword, can you use something like this instead?

const tokenBeforeRightParen = sourceCode.getLastToken(node); !astUtils.canTokensBeAdjacent(tokenBeforeRightParen, tokenAfterRightParen)

The reason a space is needed isn't because the following token is a keyword, it's because the two tokens will combine if the parens are removed. This makes a difference in cases like this:

for ((foo['bar'])of baz);

The ] and of would not combine into a single token if the parens were removed, but the autofixer unnecessarily inserts a space at the moment.

…8393)

eslintbot · 2017-06-03T12:57:25Z

LGTM

eslintbot · 2017-06-03T12:59:53Z

LGTM

eslintbot · 2017-06-03T13:03:32Z

LGTM

VictorHom · 2017-06-03T13:06:37Z

@not-an-aardvark thanks

I added the example you provided as a test if that is alright:
for ((foo['bar'])of baz);

I updated to check for adjacent nodes as you suggested. Let me know if there are other updates!

kaicataldo

Thanks for contributing to ESLint! Just some minor nitpicks, but otherwise LGTM.

kaicataldo · 2017-06-06T22:33:48Z

tests/lib/rules/no-extra-parens.js

@@ -1005,3 +1025,5 @@ ruleTester.run("no-extra-parens", rule, {
        )
    ]
 });
+
+


Nit: Extra newline here

As an aside, maybe we should enforce this with no-multiple-empty-lines on the codebase.

Yes, agreed!

kaicataldo · 2017-06-06T22:36:51Z

lib/rules/no-extra-parens.js

+         * @returns {boolean} `true` if a space should be inserted after the node
+         * @private
+         */
+        function requiresAfterSpace(node) {


Nitpick: requiresTrailingSpace might be a little clearer for this function name

not-an-aardvark

Looks good to me once @kaicataldo's comments are addressed. Thanks!

eslintbot · 2017-06-07T01:32:53Z

LGTM

VictorHom · 2017-06-07T01:34:37Z

@kaicataldo @not-an-aardvark addressed the comments.

kaicataldo

LGTM. Thanks for contributing to ESLint!

(refs #8679 (comment))

Fix: Add support for parens on left side for-loops (fixes: eslint#8393)

4731ca7

VictorHom mentioned this pull request Jun 2, 2017

no-extra-parens has a false negative in the left side of for loops #8393

Closed

not-an-aardvark suggested changes Jun 3, 2017

View reviewed changes

not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules labels Jun 3, 2017

VictorHom changed the title ~~Fix: (WIP) Add support for parens on left side for-loops (fixes: #8393)~~ Update: (WIP) Add support for parens on left side for-loops (fixes: #8393) Jun 3, 2017

Update: Add support for parens on left side for-loops (fixes: eslint#…

4e7aa1a

…8393)

VictorHom force-pushed the pr/false_negative_no_exta_parens branch from 4731ca7 to 0e7a05d Compare June 3, 2017 12:57

VictorHom force-pushed the pr/false_negative_no_exta_parens branch from 0e7a05d to e193bdf Compare June 3, 2017 12:59

VictorHom force-pushed the pr/false_negative_no_exta_parens branch from e193bdf to 4e7aa1a Compare June 3, 2017 13:03

VictorHom changed the title ~~Update: (WIP) Add support for parens on left side for-loops (fixes: #8393)~~ Update: Add support for parens on left side for-loops (fixes: #8393) Jun 5, 2017

kaicataldo reviewed Jun 6, 2017

View reviewed changes

not-an-aardvark approved these changes Jun 6, 2017

View reviewed changes

change func name to requiresTrailingSpace

96d94db

kaicataldo approved these changes Jun 7, 2017

View reviewed changes

kaicataldo merged commit 34c4020 into eslint:master Jun 7, 2017

not-an-aardvark added a commit that referenced this pull request Jun 7, 2017

Chore: enable no-multiple-empty-lines on ESLint codebase

c212b19

(refs #8679 (comment))

not-an-aardvark mentioned this pull request Jun 7, 2017

Chore: enable no-multiple-empty-lines on ESLint codebase #8694

Merged

not-an-aardvark added a commit that referenced this pull request Jun 7, 2017

Chore: enable no-multiple-empty-lines on ESLint codebase

c40a65c

(refs #8679 (comment))

not-an-aardvark added a commit that referenced this pull request Jun 8, 2017

Chore: enable no-multiple-empty-lines on ESLint codebase (#8694)

c702858

(refs #8679 (comment))

renovate bot mentioned this pull request Jun 12, 2017

Update dependency eslint to version 4.x renovatebot/renovate#290

Merged

renovate-bot mentioned this pull request Jun 15, 2017

Renovate eslint packages singapore/lint-condo#308

Merged

This was referenced Jun 23, 2017

Update dependency eslint to version 4.x homezen/hz-test-helpers#42

Merged

Update dependency eslint to version 4.x xDae/styled-bootstrap#54

Closed

chore(deps): Update dependency eslint to version 4.x renovatebot/renovate#396

Merged

This was referenced Jul 7, 2017

Update dependency eslint to version 4.x algolia/react-element-to-jsx-string#94

Closed

chore(deps): update eslint to v4.x luftywiranda13/generator-bunny#161

Closed

chore(deps): update eslint to v4.x luftywiranda13/generator-bunny#162

Closed

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: Add support for parens on left side for-loops (fixes: #8393) #8679

Update: Add support for parens on left side for-loops (fixes: #8393) #8679

VictorHom commented Jun 2, 2017 •

edited

eslintbot commented Jun 2, 2017

mention-bot commented Jun 2, 2017

not-an-aardvark left a comment

not-an-aardvark Jun 3, 2017

eslintbot commented Jun 3, 2017

eslintbot commented Jun 3, 2017

eslintbot commented Jun 3, 2017

VictorHom commented Jun 3, 2017

kaicataldo left a comment

kaicataldo Jun 6, 2017

not-an-aardvark Jun 6, 2017 •

edited

kaicataldo Jun 7, 2017

kaicataldo Jun 6, 2017

not-an-aardvark left a comment

eslintbot commented Jun 7, 2017

VictorHom commented Jun 7, 2017 •

edited

kaicataldo left a comment

@@ @@ -1005,3 +1025,5 @@ ruleTester.run("no-extra-parens", rule, { @@
                       )
                   ]
               });

Update: Add support for parens on left side for-loops (fixes: #8393) #8679

Update: Add support for parens on left side for-loops (fixes: #8393) #8679

Conversation

VictorHom commented Jun 2, 2017 • edited

eslintbot commented Jun 2, 2017

mention-bot commented Jun 2, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

not-an-aardvark Jun 3, 2017

Choose a reason for hiding this comment

eslintbot commented Jun 3, 2017

eslintbot commented Jun 3, 2017

eslintbot commented Jun 3, 2017

VictorHom commented Jun 3, 2017

kaicataldo left a comment

Choose a reason for hiding this comment

kaicataldo Jun 6, 2017

Choose a reason for hiding this comment

not-an-aardvark Jun 6, 2017 • edited

Choose a reason for hiding this comment

kaicataldo Jun 7, 2017

Choose a reason for hiding this comment

kaicataldo Jun 6, 2017

Choose a reason for hiding this comment

not-an-aardvark left a comment

Choose a reason for hiding this comment

eslintbot commented Jun 7, 2017

VictorHom commented Jun 7, 2017 • edited

kaicataldo left a comment

Choose a reason for hiding this comment

VictorHom commented Jun 2, 2017 •

edited

not-an-aardvark Jun 6, 2017 •

edited

VictorHom commented Jun 7, 2017 •

edited