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

Update: Add consistent option to computed-property-spacing #12406

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions docs/developer-guide/working-with-rules.md
Expand Up @@ -391,6 +391,8 @@ Once you have an instance of `SourceCode`, you can use the methods on it to work
* `getCommentsAfter(nodeOrToken)` - returns an array of comment tokens that occur directly after the given node or token.
* `getCommentsInside(node)` - returns an array of all comment tokens inside a given node.
* `getJSDocComment(node)` - returns the JSDoc comment for a given node or `null` if there is none.
* `getTextBetweenTokens(first, second)` - returns text between two tokens.
* `getCleanTextBetweenTokens(first, second)` - returns text between two tokens without inline comments (`/* */`).
* `isSpaceBetweenTokens(first, second)` - returns true if there is a whitespace character between the two tokens.
* `getFirstToken(node, skipOptions)` - returns the first token representing the given node.
* `getFirstTokens(node, countOptions)` - returns the first `count` tokens representing the given node.
Expand Down
58 changes: 58 additions & 0 deletions docs/rules/computed-property-spacing.md
Expand Up @@ -31,10 +31,12 @@ String option:

* `"never"` (default) disallows spaces inside computed property brackets
* `"always"` requires one or more spaces inside computed property brackets
* `"consistent"` requires consistent usage of spaces inside computed property brackets: there should be equal number of spaces after `[` and before `]` (the limit of spaces can be set by `maxSpaces` option, see below)

Object option:

* `"enforceForClassMembers": true` additionally applies this rule to class members (default is `false`)
* `"maxSpaces": 2` specifies maximum number of allowed spaced for `"consistent"` option (default is `1`)

### never

Expand Down Expand Up @@ -90,6 +92,34 @@ var x = {[ b ]: a}
obj[ foo[ bar ] ]
```

### consistent

Examples of **incorrect** code for this rule with the `"consistent"` option:

```js
/*eslint computed-property-spacing: ["error", "consistent"]*/
/*eslint-env es6*/

obj[foo ]
var x = {[ b]: a}
obj[ foo ]
obj[ 'foo' ]
obj[ foo[ bar ]]
var x = {[b ]: a}
```

Examples of **correct** code for this rule with the `"consistent"` option:

```js
/*eslint computed-property-spacing: ["error", "consistent"]*/
/*eslint-env es6*/

obj[ foo ]
obj['foo']
var x = {[ b ]: a[ c[d] ]}
obj[ foo[bar] ]
```

#### enforceForClassMembers

By default, this rule does not check class declarations and class expressions,
Expand Down Expand Up @@ -138,6 +168,34 @@ const Bar = class {
}
```

### maxSpaces

Examples of **incorrect** code for this rule with `"consistent"` and `{ maxSpaces: 2 }`:

```js
/*eslint computed-property-spacing: ["error", "consistent", { "maxSpaces": 2 }]*/
/*eslint-env es6*/

obj[ foo ]
var x = {[ b ]: a}
obj[ foo ]
obj['foo' ]
obj[ foo[ bar ] ]
var x = {[ b ]: a}
```

Examples of **correct** code for this rule with `"consistent"` and `{ maxSpaces: 2 }`:

```js
/*eslint computed-property-spacing: ["error", "consistent", { "maxSpaces": 2 }]*/
/*eslint-env es6*/

obj[ foo ]
obj[ 'foo' ]
var x = {[ b ]: a[ c[ d ] ]}
obj[ foo[bar] ]
```

## When Not To Use It

You can turn this rule off if you are not concerned with the consistency of computed properties.
Expand Down
95 changes: 74 additions & 21 deletions lib/rules/computed-property-spacing.js
Expand Up @@ -25,14 +25,19 @@ module.exports = {

schema: [
{
enum: ["always", "never"]
enum: ["always", "never", "consistent"]
},
{
type: "object",
properties: {
enforceForClassMembers: {
type: "boolean",
default: false
},
maxSpaces: {
type: "integer",
minimum: 0,
default: 1
}
},
additionalProperties: false
Expand All @@ -44,14 +49,21 @@ module.exports = {
unexpectedSpaceAfter: "There should be no space after '{{tokenValue}}'.",

missingSpaceBefore: "A space is required before '{{tokenValue}}'.",
missingSpaceAfter: "A space is required after '{{tokenValue}}'."
missingSpaceAfter: "A space is required after '{{tokenValue}}'.",

inconsistentSpaces: "There should be equal number (no more than {{maxSpaces}}) of spaces around '{{expression}}'."
}
},

create(context) {
const sourceCode = context.getSourceCode();
const propertyNameMustBeSpaced = context.options[0] === "always"; // default is "never"
const enforceForClassMembers = context.options[1] && context.options[1].enforceForClassMembers;
const options = context.options;
const checkType = options[0];
const settings = options[1] || {};
const propertyNameMustBeSpaced = checkType === "always"; // default is "never"
const consistentSpaces = checkType === "consistent";
const enforceForClassMembers = settings.enforceForClassMembers;
const maxSpaces = ("maxSpaces" in settings) ? settings.maxSpaces : 1;

//--------------------------------------------------------------------------
// Helpers
Expand Down Expand Up @@ -139,6 +151,33 @@ module.exports = {
});
}

/**
* Reports that there should be equal number of spaces around expression.
* @param {ASTNode} node The node to report in the event of an error.
* @param {Token} beforeToken The token before expression.
* @param {Token} firstToken The first token of expression.
* @param {Token} lastToken The last token of expression.
* @param {Token} afterToken The token after expression.
* @returns {void}
*/
function reportInconsistentSpaces(node, beforeToken, firstToken, lastToken, afterToken) {
context.report({
node,
loc: firstToken.loc.start,
messageId: "inconsistentSpaces",
data: {
expression: sourceCode.getCleanTextBetweenTokens(beforeToken, afterToken).trim(),
maxSpaces
},
fix(fixer) {
return [
fixer.removeRange([beforeToken.range[1], firstToken.range[0]]),
fixer.removeRange([lastToken.range[1], afterToken.range[0]])
];
}
});
}

/**
* Returns a function that checks the spacing of a node on the property name
* that was passed in.
Expand All @@ -156,28 +195,42 @@ module.exports = {
const before = sourceCode.getTokenBefore(property),
first = sourceCode.getFirstToken(property),
last = sourceCode.getLastToken(property),
after = sourceCode.getTokenAfter(property);
after = sourceCode.getTokenAfter(property),
beforeFirstOnSameLine = astUtils.isTokenOnSameLine(before, first),
lastAfterOnSameLine = astUtils.isTokenOnSameLine(last, after);
let textBetweenBeforeFirst,
textBetweenLastAfter;

if (astUtils.isTokenOnSameLine(before, first)) {
if (propertyNameMustBeSpaced) {
if (!sourceCode.isSpaceBetweenTokens(before, first) && astUtils.isTokenOnSameLine(before, first)) {
reportRequiredBeginningSpace(node, before);
if (consistentSpaces) {
if (beforeFirstOnSameLine || lastAfterOnSameLine) {
textBetweenBeforeFirst = sourceCode.getCleanTextBetweenTokens(before, first);
textBetweenLastAfter = sourceCode.getCleanTextBetweenTokens(last, after);
if (textBetweenBeforeFirst !== textBetweenLastAfter || !/^ *$/u.test(textBetweenLastAfter) || textBetweenLastAfter.length > maxSpaces) {
reportInconsistentSpaces(node, before, first, last, after);
}
} else {
if (sourceCode.isSpaceBetweenTokens(before, first)) {
reportNoBeginningSpace(node, before, first);
}
} else {
if (beforeFirstOnSameLine) {
if (propertyNameMustBeSpaced) {
if (!sourceCode.isSpaceBetweenTokens(before, first)) {
reportRequiredBeginningSpace(node, before);
}
} else {
if (sourceCode.isSpaceBetweenTokens(before, first)) {
reportNoBeginningSpace(node, before, first);
}
}
}
}

if (astUtils.isTokenOnSameLine(last, after)) {
if (propertyNameMustBeSpaced) {
if (!sourceCode.isSpaceBetweenTokens(last, after) && astUtils.isTokenOnSameLine(last, after)) {
reportRequiredEndingSpace(node, after);
}
} else {
if (sourceCode.isSpaceBetweenTokens(last, after)) {
reportNoEndingSpace(node, after, last);
if (lastAfterOnSameLine) {
if (propertyNameMustBeSpaced) {
if (!sourceCode.isSpaceBetweenTokens(last, after)) {
reportRequiredEndingSpace(node, after);
}
} else {
if (sourceCode.isSpaceBetweenTokens(last, after)) {
reportNoEndingSpace(node, after, last);
}
}
}
}
Expand Down
36 changes: 33 additions & 3 deletions lib/source-code/source-code.js
Expand Up @@ -193,6 +193,16 @@ class SourceCode extends TokenStore {
Object.freeze(this.lines);
}

/**
* Removes inline comments (`\/* *\/`) from specified text.
* @param {string} text Text to process.
* @returns {string} Text without inline comments.
* @public
*/
static removeInlineComments(text) {
return text.replace(/\/\*.*?\*\//gu, "");
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this it's a good idea to add this to the public API. This check only works when given the text between two adjacent tokens. If allowed any arbitrary text, a string containing this pattern could be mutated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As stated in name and comment this method is intended to remove inline comments i.e. /* */ comments inside a line. So it is quite generic and can be helpful (at least in this module).

But I can replace the method by private function.

Copy link
Member

@kaicataldo kaicataldo Nov 1, 2019

Choose a reason for hiding this comment

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

I understand. However, this would also mutate the following text:

const constructComment = `/*${insertCommentText()}*/`;

becomes

const constructComment = ``;

I actually think the code this is being lifted from in isSpaceBetweenTokens is also buggy, and there is a PR open to fix it.

}

/**
* Split the source code into multiple lines based on the line delimiters.
* @param {string} text Source code as a string.
Expand Down Expand Up @@ -410,6 +420,28 @@ class SourceCode extends TokenStore {
return result;
}

/**
* Returns text between two tokens.
* @param {Token} first The token to get text after.
* @param {Token} second The token to get text before.
* @returns {string} Text between specified tokens.
* @public
*/
getTextBetweenTokens(first, second) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not opposed to adding this to our public API, but I think the other two added methods should be implemented in rules or, if shared, in ast-utils.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These methods may be useful for other rules too.
I'm not sure about moving these methods into ast-utils.
I've extracted code for methods from isSpaceBetweenTokens and made the code reusable.
If considering this idea, then isSpaceBetweenTokens can also be moved into ast-utils.

return this.text.slice(first.range[1], second.range[0]);
}

/**
* Returns text between two tokens without inline comments.
* @param {Token} first The token to get text after.
* @param {Token} second The token to get text before.
* @returns {string} Text between specified tokens without inline comments.
* @public
*/
getCleanTextBetweenTokens(first, second) {
return SourceCode.removeInlineComments(this.getTextBetweenTokens(first, second));
}

/**
* Determines if two tokens have at least one whitespace character
* between them. This completely disregards comments in making the
Expand All @@ -421,9 +453,7 @@ class SourceCode extends TokenStore {
* @public
*/
isSpaceBetweenTokens(first, second) {
const text = this.text.slice(first.range[1], second.range[0]);

return /\s/u.test(text.replace(/\/\*.*?\*\//gu, ""));
return /\s/u.test(this.getCleanTextBetweenTokens(first, second));
}

/**
Expand Down