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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from Jun 7, 2017
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4731ca7
Fix: Add support for parens on left side for-loops (fixes: #8393)
VictorHom Jun 2, 2017
4e7aa1a
Update: Add support for parens on left side for-loops (fixes: #8393)
VictorHom Jun 3, 2017
96d94db
change func name to requiresTrailingSpace
VictorHom Jun 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 23 additions & 1 deletion lib/rules/no-extra-parens.js
View file
Open in desktop
Expand Up @@ -255,6 +255,22 @@ module.exports = {
!astUtils.canTokensBeAdjacent(tokenBeforeLeftParen, firstToken);
}

/**
* Determines whether a node should be followed by an additional space when removing parens
* @param {ASTNode} node node to evaluate; must be surrounded by parentheses
* @returns {boolean} `true` if a space should be inserted after the node
* @private
*/
function requiresAfterSpace(node) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const nextTwoTokens = sourceCode.getTokensAfter(node, { count: 2 });
const rightParenToken = nextTwoTokens[0];
const tokenAfterRightParen = nextTwoTokens[1];

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
* Report the node
* @param {ASTNode} node node to evaluate
Expand All @@ -279,7 +295,7 @@ module.exports = {
return fixer.replaceTextRange([
leftParenToken.range[0],
rightParenToken.range[1]
], (requiresLeadingSpace(node) ? " " : "") + parenthesizedSource);
], (requiresLeadingSpace(node) ? " " : "") + parenthesizedSource + (requiresAfterSpace(node) ? " " : ""));
}
});
}
Expand Down Expand Up @@ -488,12 +504,18 @@ module.exports = {
if (hasExcessParens(node.right)) {
report(node.right);
}
if (hasExcessParens(node.left)) {
report(node.left);
}
},

ForOfStatement(node) {
if (hasExcessParens(node.right)) {
report(node.right);
}
if (hasExcessParens(node.left)) {
report(node.left);
}
},

ForStatement(node) {
Expand Down
15 changes: 14 additions & 1 deletion tests/lib/rules/no-extra-parens.js
View file
Open in desktop
Expand Up @@ -41,7 +41,6 @@ function invalid(code, output, type, line, config) {
if (line) {
result.errors[0].line = line;
}

return result;
}

Expand Down Expand Up @@ -996,6 +995,20 @@ ruleTester.run("no-extra-parens", rule, {
1,
{ parserOptions: { ecmaVersion: 2015 } }
),
invalid(
"for ((foo) of bar);",
"for (foo of bar);",
"Identifier",
1,
{ parserOptions: { ecmaVersion: 2015 } }
),
invalid(
"for ((foo)in bar);",
"for (foo in bar);",
"Identifier",
1,
{ parserOptions: { ecmaVersion: 2015 } }
),
invalid(
"() => (({ foo: 1 }).foo)",
"() => ({ foo: 1 }).foo",
Expand Down