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

New: add option first for VariableDeclarator in indent (fixes #8976) #11193

Merged
merged 3 commits into from Dec 23, 2018
Merged

New: add option first for VariableDeclarator in indent (fixes #8976) #11193

merged 3 commits into from Dec 23, 2018

Conversation

@g-plane
Copy link
Member

@g-plane g-plane commented Dec 14, 2018

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 first or not. If the option isn't first, it won't execute the addElementListIndent function to make sure not breaking the current rule logic and not doing unnecessary execution of the addElementListIndent function.

Here is an online demo: https://eslint.gplane.win/pr/11193/#eJwdybsKgDAMRuFX+ckoBcGxrj6Di3GINUKhRIjFRXx3L8sZvnNR2lelSG2jR8lWkW1VqxETk7rvzhTQBVxgGsWzLEUHTUVc6jfj61v2ozLhnpuWje0UhwQ2YPmbeja6H2LUIqg=

Copy link
Member

@aladdin-add aladdin-add left a comment

LGTM, it seems much more elegant than I thought, thanks!

Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Thanks for the PR! I left a few suggestions.

@@ -1385,6 +1373,15 @@ module.exports = {
if (astUtils.isSemicolonToken(lastToken)) {
offsets.ignoreToken(lastToken);
}

if (options.VariableDeclarator[node.kind] === "first") {
Copy link
Member

@not-an-aardvark not-an-aardvark Dec 16, 2018

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.

addElementListIndent(
node.declarations,
sourceCode.getFirstToken(node),
sourceCode.getLastToken(node),
Copy link
Member

@not-an-aardvark not-an-aardvark Dec 16, 2018

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,
    baz
let
    foo
let foo = 1,
    bar =
    2

Copy link
Member Author

@g-plane g-plane Dec 16, 2018

Do you expect the cases above are reporting warnings or not?

Copy link
Member

@not-an-aardvark not-an-aardvark Dec 16, 2018

No, they should be valid.

Copy link
Member Author

@g-plane g-plane Dec 16, 2018

I have added those test cases. See the latest commit please.

Copy link
Member

@platinumazure platinumazure left a comment

Just one possible small simplification, otherwise this looks good to me! Thanks!

const firstToken = sourceCode.getFirstToken(node),
lastToken = sourceCode.getLastToken(node);

if (options.VariableDeclarator[node.kind] === "first") {
Copy link
Member

@platinumazure platinumazure Dec 22, 2018

Can this be simplified?

if (variableIndent === "first" && node.declarations.length > 1)

Might also allow the let a few lines up to revert to const.

Copy link
Member Author

@g-plane g-plane Dec 23, 2018

It may not. You can see there is a line (line 1358) after the if statement (node.declarations.length > 1).

Copy link
Member

@platinumazure platinumazure Dec 23, 2018

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.

Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

LGTM, thanks!

@platinumazure platinumazure merged commit b4395f6 into eslint:master Dec 23, 2018
5 checks passed
@g-plane g-plane deleted the fix-8976 branch Dec 23, 2018
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jun 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants