-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Fix: Support type annotations in array-bracket-spacing #7445
Fix: Support type annotations in array-bracket-spacing #7445
Conversation
LGTM |
@taion, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @alberto and @mysticatea to be potential reviewers. |
Thank you for the PR, @taion! Could you fill the bug fix/report template too? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for contributing!
But I'm not sure that those tests are legal.
Could you confirm the language spec?
|
||
// destructuring with type annotation | ||
{ code: "([ 1, 1 ]: Array<any>)", options: ["always"], parserOptions: { ecmaVersion: 6 }, parser: parser("flow-destructuring-1") }, | ||
{ code: "([1, 1]: Array< any >)", options: ["never"], parserOptions: { ecmaVersion: 6 }, parser: parser("flow-destructuring-2") }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this valid syntax?
As far as I know, the array patterns of destructuring cannot exist independently.
- It needs the initializer of variable declarations: e.g.
let [a] = b
- It needs to be at LHS of assignments: e.g.
([a] = b)
- It needs to be in parameters: e.g.
([a]) => a
Also, those AST looks ArrayExpression
, not ArrayPattern
. Those are not destructuring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found it, those tests are Typecasts syntax.
Those ArrayExpression
nodes don't have typeAnnotation
s, so those tests are not proper.
I was overzealous in cutting down the test case. Will replace. |
LGTM |
Updated. The test should be correct now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Tests look good to me.
penultimate = sourceCode.getLastToken(node, 1), | ||
last = sourceCode.getLastToken(node), | ||
last = node.typeAnnotation | ||
? sourceCode.getFirstToken(node.typeAnnotation, -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use sourceCode.getTokenBefore(node.typeAnnotation)
instead?
LGTM |
Switched to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
penultimate = sourceCode.getLastToken(node, 1), | ||
last = sourceCode.getLastToken(node), | ||
last = node.typeAnnotation | ||
? sourceCode.getTokenBefore(node.typeAnnotation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering, can this be made into a sourceCode.getTokenAfter(/* last element */)
and then work for both cases? But this looks perfectly fine to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to do that – the problem is that node.elements
is an empty array if there are no elements, so there may not be a last element. It turns out that
([]: any) => {}
is actually legal. Deeply silly, yes, but actually a thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess sourceCode.getTokenAfter(/* last element */)
does not work if there is a trailing comma, or if no item (e.g. let [] = a;
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points. Let's leave it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for contributing!
What is the purpose of this pull request? (put an "X" next to item)
[X] Bug fix (template)
Original issue: #4753
Per
comma-dangle
,object-curly-spacing
, and #7436, it appears that these updates can belong in ESLint now.What changes did you make? (Give an overview)
In
array-bracket-spacing
, if a type annotation is present, take the last token and the penultimate token before the type annotation.Is there anything you'd like reviewers to focus on?
This code is slightly different from the code in
eslint-plugin-babel
. The code there just steps backward until it finds a]
. Here I instead look at the token immediately before the beginning of the type annotation. This avoids issues when the type annotation is of the formType[]
.cc @zaygraveyard