From bfe5e884098874bb512609bcd94a5e5ed797839d Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Fri, 8 Jul 2022 02:48:21 +0200 Subject: [PATCH] fix: ignore spacing before `]` and `}` in comma-spacing (#16113) This avoids conflicts with array-bracket-spacing and object-curly-spacing rules. Fixes #16100 --- docs/src/rules/comma-spacing.md | 48 ++++++++++++-------- lib/rules/comma-spacing.js | 76 +++++++++++++++----------------- tests/lib/rules/comma-spacing.js | 55 ++++++++++++++++++----- 3 files changed, 107 insertions(+), 72 deletions(-) diff --git a/docs/src/rules/comma-spacing.md b/docs/src/rules/comma-spacing.md index ed4bae760c5..ca10ca0a6a7 100644 --- a/docs/src/rules/comma-spacing.md +++ b/docs/src/rules/comma-spacing.md @@ -6,6 +6,7 @@ rule_type: layout related_rules: - array-bracket-spacing - comma-style +- object-curly-spacing - space-in-brackets - space-in-parens - space-infix-ops @@ -30,10 +31,13 @@ var foo = 1 ,bar = 2; This rule enforces consistent spacing before and after commas in variable declarations, array literals, object literals, function parameters, and sequences. -This rule does not apply in an `ArrayExpression` or `ArrayPattern` in either of the following cases: +This rule does not apply in either of the following cases: -* adjacent null elements -* an initial null element, to avoid conflicts with the [`array-bracket-spacing`](array-bracket-spacing) rule +* between two commas +* between opening bracket `[` and comma, to avoid conflicts with the [`array-bracket-spacing`](array-bracket-spacing) rule +* between comma and closing bracket `]`, to avoid conflicts with the [`array-bracket-spacing`](array-bracket-spacing) rule +* between comma and closing brace `}`, to avoid conflicts with the [`object-curly-spacing`](object-curly-spacing) rule +* between comma and closing parentheses `)`, to avoid conflicts with the [`space-in-parens`](space-in-parens) rule ## Options @@ -84,15 +88,34 @@ a, b ::: -Example of **correct** code for this rule with initial null element for the default `{ "before": false, "after": true }` options: +Additional examples of **correct** code for this rule with the default `{ "before": false, "after": true }` options: :::correct ```js /*eslint comma-spacing: ["error", { "before": false, "after": true }]*/ -/*eslint array-bracket-spacing: ["error", "always"]*/ -var arr = [ , 2, 3 ] +// this rule does not enforce spacing between two commas +var arr = [ + ,, + , , +]; + +// this rule does not enforce spacing after `[` and before `]` +var arr = [,]; +var arr = [ , ]; +var arr = [a, b,]; +[,] = arr; +[ , ] = arr; +[a, b,] = arr; + +// this rule does not enforce spacing before `}` +var obj = {x, y,}; +var {z, q,} = obj; +import {foo, bar,} from "mod"; + +// this rule does not enforce spacing before `)` +foo(a, b,) ``` ::: @@ -136,19 +159,6 @@ a ,b ::: -Examples of **correct** code for this rule with initial null element for the `{ "before": true, "after": false }` options: - -:::correct - -```js -/*eslint comma-spacing: ["error", { "before": true, "after": false }]*/ -/*eslint array-bracket-spacing: ["error", "never"]*/ - -var arr = [,2 ,3] -``` - -::: - ## When Not To Use It If your project will not be following a consistent comma-spacing pattern, turn this rule off. diff --git a/lib/rules/comma-spacing.js b/lib/rules/comma-spacing.js index 8cc8059b5ff..76d5dc46b9c 100644 --- a/lib/rules/comma-spacing.js +++ b/lib/rules/comma-spacing.js @@ -103,38 +103,6 @@ module.exports = { }); } - /** - * Validates the spacing around a comma token. - * @param {Object} tokens The tokens to be validated. - * @param {Token} tokens.comma The token representing the comma. - * @param {Token} [tokens.left] The last token before the comma. - * @param {Token} [tokens.right] The first token after the comma. - * @param {Token|ASTNode} reportItem The item to use when reporting an error. - * @returns {void} - * @private - */ - function validateCommaItemSpacing(tokens, reportItem) { - if (tokens.left && astUtils.isTokenOnSameLine(tokens.left, tokens.comma) && - (options.before !== sourceCode.isSpaceBetweenTokens(tokens.left, tokens.comma)) - ) { - report(reportItem, "before", tokens.left); - } - - if (tokens.right && astUtils.isClosingParenToken(tokens.right)) { - return; - } - - if (tokens.right && !options.after && tokens.right.type === "Line") { - return; - } - - if (tokens.right && astUtils.isTokenOnSameLine(tokens.comma, tokens.right) && - (options.after !== sourceCode.isSpaceBetweenTokens(tokens.comma, tokens.right)) - ) { - report(reportItem, "after", tokens.right); - } - } - /** * Adds null elements of the given ArrayExpression or ArrayPattern node to the ignore list. * @param {ASTNode} node An ArrayExpression or ArrayPattern node. @@ -172,18 +140,44 @@ module.exports = { return; } - if (token && token.type === "JSXText") { - return; - } - const previousToken = tokensAndComments[i - 1]; const nextToken = tokensAndComments[i + 1]; - validateCommaItemSpacing({ - comma: token, - left: astUtils.isCommaToken(previousToken) || commaTokensToIgnore.includes(token) ? null : previousToken, - right: astUtils.isCommaToken(nextToken) ? null : nextToken - }, token); + if ( + previousToken && + !astUtils.isCommaToken(previousToken) && // ignore spacing between two commas + + /* + * `commaTokensToIgnore` are ending commas of `null` elements (array holes/elisions). + * In addition to spacing between two commas, this can also ignore: + * + * - Spacing after `[` (controlled by array-bracket-spacing) + * Example: [ , ] + * ^ + * - Spacing after a comment (for backwards compatibility, this was possibly unintentional) + * Example: [a, /* * / ,] + * ^ + */ + !commaTokensToIgnore.includes(token) && + + astUtils.isTokenOnSameLine(previousToken, token) && + options.before !== sourceCode.isSpaceBetweenTokens(previousToken, token) + ) { + report(token, "before", previousToken); + } + + if ( + nextToken && + !astUtils.isCommaToken(nextToken) && // ignore spacing between two commas + !astUtils.isClosingParenToken(nextToken) && // controlled by space-in-parens + !astUtils.isClosingBracketToken(nextToken) && // controlled by array-bracket-spacing + !astUtils.isClosingBraceToken(nextToken) && // controlled by object-curly-spacing + !(!options.after && nextToken.type === "Line") && // special case, allow space before line comment + astUtils.isTokenOnSameLine(token, nextToken) && + options.after !== sourceCode.isSpaceBetweenTokens(token, nextToken) + ) { + report(token, "after", nextToken); + } }); }, ArrayExpression: addNullElementsToIgnoreList, diff --git a/tests/lib/rules/comma-spacing.js b/tests/lib/rules/comma-spacing.js index df7dac6324a..a3b23b62d73 100644 --- a/tests/lib/rules/comma-spacing.js +++ b/tests/lib/rules/comma-spacing.js @@ -27,19 +27,36 @@ ruleTester.run("comma-spacing", rule, { "myfunc(404, // comment\n true, /* bla bla bla */ 'hello');", { code: "myfunc(404, // comment\n true,/* bla bla bla */ 'hello');", options: [{ before: false, after: false }] }, "var a = 1, b = 2;", + "var arr = [,];", "var arr = [, ];", + "var arr = [ ,];", + "var arr = [ , ];", + "var arr = [1,];", "var arr = [1, ];", "var arr = [, 2];", + "var arr = [ , 2];", "var arr = [1, 2];", + "var arr = [,,];", + "var arr = [ ,,];", + "var arr = [, ,];", + "var arr = [,, ];", + "var arr = [ , ,];", + "var arr = [ ,, ];", "var arr = [, , ];", + "var arr = [ , , ];", "var arr = [1, , ];", "var arr = [, 2, ];", "var arr = [, , 3];", + "var arr = [,, 3];", "var arr = [1, 2, ];", "var arr = [, 2, 3];", "var arr = [1, , 3];", "var arr = [1, 2, 3];", + "var arr = [1, 2, 3,];", + "var arr = [1, 2, 3, ];", "var obj = {'foo':'bar', 'baz':'qur'};", + "var obj = {'foo':'bar', 'baz':'qur', };", + "var obj = {'foo':'bar', 'baz':'qur',};", "var obj = {'foo':'bar', 'baz':\n'qur'};", "var obj = {'foo':\n'bar', 'baz':\n'qur'};", "function foo(a, b){}", @@ -74,20 +91,34 @@ ruleTester.run("comma-spacing", rule, { { code: "var a = 1 ,b = 2;", options: [{ before: true, after: false }] }, { code: "function foo(a ,b){}", options: [{ before: true, after: false }] }, { code: "var arr = [,];", options: [{ before: true, after: false }] }, + { code: "var arr = [ ,];", options: [{ before: true, after: false }] }, + { code: "var arr = [, ];", options: [{ before: true, after: false }] }, + { code: "var arr = [ , ];", options: [{ before: true, after: false }] }, { code: "var arr = [1 ,];", options: [{ before: true, after: false }] }, + { code: "var arr = [1 , ];", options: [{ before: true, after: false }] }, { code: "var arr = [ ,2];", options: [{ before: true, after: false }] }, { code: "var arr = [1 ,2];", options: [{ before: true, after: false }] }, { code: "var arr = [,,];", options: [{ before: true, after: false }] }, + { code: "var arr = [ ,,];", options: [{ before: true, after: false }] }, + { code: "var arr = [, ,];", options: [{ before: true, after: false }] }, + { code: "var arr = [,, ];", options: [{ before: true, after: false }] }, + { code: "var arr = [ , ,];", options: [{ before: true, after: false }] }, + { code: "var arr = [ ,, ];", options: [{ before: true, after: false }] }, + { code: "var arr = [, , ];", options: [{ before: true, after: false }] }, + { code: "var arr = [ , , ];", options: [{ before: true, after: false }] }, { code: "var arr = [1 , ,];", options: [{ before: true, after: false }] }, { code: "var arr = [ ,2 ,];", options: [{ before: true, after: false }] }, + { code: "var arr = [,2 , ];", options: [{ before: true, after: false }] }, { code: "var arr = [ , ,3];", options: [{ before: true, after: false }] }, { code: "var arr = [1 ,2 ,];", options: [{ before: true, after: false }] }, { code: "var arr = [ ,2 ,3];", options: [{ before: true, after: false }] }, { code: "var arr = [1 , ,3];", options: [{ before: true, after: false }] }, { code: "var arr = [1 ,2 ,3];", options: [{ before: true, after: false }] }, { code: "var obj = {'foo':'bar' , 'baz':'qur'};", options: [{ before: true, after: true }] }, + { code: "var obj = {'foo':'bar' ,'baz':'qur' , };", options: [{ before: true, after: false }] }, { code: "var a = 1 , b = 2;", options: [{ before: true, after: true }] }, { code: "var arr = [, ];", options: [{ before: true, after: true }] }, + { code: "var arr = [,,];", options: [{ before: true, after: true }] }, { code: "var arr = [1 , ];", options: [{ before: true, after: true }] }, { code: "var arr = [ , 2];", options: [{ before: true, after: true }] }, { code: "var arr = [1 , 2];", options: [{ before: true, after: true }] }, @@ -107,6 +138,7 @@ ruleTester.run("comma-spacing", rule, { { code: "var arr = [ ,2];", options: [{ before: false, after: false }] }, { code: "var arr = [1,2];", options: [{ before: false, after: false }] }, { code: "var arr = [,,];", options: [{ before: false, after: false }] }, + { code: "var arr = [ , , ];", options: [{ before: false, after: false }] }, { code: "var arr = [ ,,];", options: [{ before: false, after: false }] }, { code: "var arr = [1,,];", options: [{ before: false, after: false }] }, { code: "var arr = [,2,];", options: [{ before: false, after: false }] }, @@ -120,12 +152,22 @@ ruleTester.run("comma-spacing", rule, { { code: "var a; console.log(`${a}`, \"a\");", parserOptions: { ecmaVersion: 6 } }, { code: "var [a, b] = [1, 2];", parserOptions: { ecmaVersion: 6 } }, { code: "var [a, b, ] = [1, 2];", parserOptions: { ecmaVersion: 6 } }, + { code: "var [a, b,] = [1, 2];", parserOptions: { ecmaVersion: 6 } }, { code: "var [a, , b] = [1, 2, 3];", parserOptions: { ecmaVersion: 6 } }, + { code: "var [a,, b] = [1, 2, 3];", parserOptions: { ecmaVersion: 6 } }, { code: "var [ , b] = a;", parserOptions: { ecmaVersion: 6 } }, { code: "var [, b] = a;", parserOptions: { ecmaVersion: 6 } }, + { code: "var { a,} = a;", parserOptions: { ecmaVersion: 6 } }, + { code: "import { a,} from 'mod';", parserOptions: { ecmaVersion: 6, sourceType: "module" } }, { code: ",", parserOptions: { ecmaVersion: 6, ecmaFeatures: { jsx: true } } }, { code: " , ", parserOptions: { ecmaVersion: 6, ecmaFeatures: { jsx: true } } }, - { code: "Hello, world", options: [{ before: true, after: false }], parserOptions: { ecmaVersion: 6, ecmaFeatures: { jsx: true } } } + { code: "Hello, world", options: [{ before: true, after: false }], parserOptions: { ecmaVersion: 6, ecmaFeatures: { jsx: true } } }, + + // For backwards compatibility. Ignoring spacing between a comment and comma of a null element was possibly unintentional. + { code: "[a, /**/ , ]", options: [{ before: false, after: true }] }, + { code: "[a , /**/, ]", options: [{ before: true, after: true }] }, + { code: "[a, /**/ , ] = foo", options: [{ before: false, after: true }], parserOptions: { ecmaVersion: 6 } }, + { code: "[a , /**/, ] = foo", options: [{ before: true, after: true }], parserOptions: { ecmaVersion: 6 } } ], invalid: [ @@ -198,17 +240,6 @@ ruleTester.run("comma-spacing", rule, { } ] }, - { - code: "var arr = [1 , ];", - output: "var arr = [1 ,];", - options: [{ before: true, after: false }], - errors: [ - { - message: "There should be no space after ','.", - type: "Punctuator" - } - ] - }, { code: "var arr = [1 ,2];", output: "var arr = [1, 2];",