New: add option first for VariableDeclarator in indent (fixes #8976)
#11193
Conversation
eslint-deprecated
bot
added
the
triage
aladdin-add
added
enhancement
lib/rules/indent.js
Outdated
| @@ -1385,6 +1373,15 @@ module.exports = { | |||
| if (astUtils.isSemicolonToken(lastToken)) { | |||
| offsets.ignoreToken(lastToken); | |||
| } | |||
|
|
|||
| if (options.VariableDeclarator[node.kind] === "first") { | |||
There was a problem hiding this comment.
For performance, it might be better to skip the lines above (starting with if (node.declarations... on line 1344) when the offset is first, because those lines would have no effect.
lib/rules/indent.js
Outdated
| addElementListIndent( | ||
| node.declarations, | ||
| sourceCode.getFirstToken(node), | ||
| sourceCode.getLastToken(node), |
There was a problem hiding this comment.
The use of sourceCode.getLastToken here seems a bit strange. Normally, when addElementListIndent, the last token is indented the same way as the first token (e.g. in [foo, bar, baz], the ] is indented the same amount as the [.
It might not be necessary to change this because the last token would be indented properly later on (e.g. in the ArrayExpression listener). However, can you add tests for cases like this?
let foo = 1,
bar = 2,
bazlet
foolet foo = 1,
bar =
2There was a problem hiding this comment.
Do you expect the cases above are reporting warnings or not?
There was a problem hiding this comment.
No, they should be valid.
There was a problem hiding this comment.
I have added those test cases. See the latest commit please.
| const firstToken = sourceCode.getFirstToken(node), | ||
| lastToken = sourceCode.getLastToken(node); | ||
|
|
||
| if (options.VariableDeclarator[node.kind] === "first") { |
There was a problem hiding this comment.
Can this be simplified?
if (variableIndent === "first" && node.declarations.length > 1)
Might also allow the let a few lines up to revert to const.
There was a problem hiding this comment.
It may not. You can see there is a line (line 1358) after the if statement (node.declarations.length > 1).
There was a problem hiding this comment.
I did see that, but for some reason I thought the original declaration also covered that case. I see now that it probably doesn't. I have no issue merging this as is.
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[x] Changes an existing rule
What changes did you make? (Give an overview)
Fixes #8976
Is there anything you'd like reviewers to focus on?
I have used a guard for checking whether the option is
firstor not. If the option isn'tfirst, it won't execute theaddElementListIndentfunction to make sure not breaking the current rule logic and not doing unnecessary execution of theaddElementListIndentfunction.Here is an online demo: https://eslint.gplane.win/pr/11193/#eJwdybsKgDAMRuFX+ckoBcGxrj6Di3GINUKhRIjFRXx3L8sZvnNR2lelSG2jR8lWkW1VqxETk7rvzhTQBVxgGsWzLEUHTUVc6jfj61v2ozLhnpuWje0UhwQ2YPmbeja6H2LUIqg=