Rewrite and simplify trailing-comma #2236
Conversation
No longer enforce trailing commas in type parameters and tuple types. Fixes: #1905 New checks for CallSignature and NamedExports. Removed buggy check for interfaces.
Report missing trailing comma at the position where it is missing with zero width instead of marking the character before. SemicolonRule does the same.
| @@ -114,7 +114,6 @@ var [ddOne, ddTwo] = dd; | |||
| var a: [string, number] = null; | |||
|
|
|||
| var a: [string, number,] = null; | |||
| ~ [Unnecessary trailing comma] | |||
There was a problem hiding this comment.
shouldn't this still be flagged?
There was a problem hiding this comment.
Actually the trailing comma in tuple types was never valid:
http://www.typescriptlang.org/play/#src=var%20a%3A%20%5Bstring%2C%20number%2C%5D%20%3D%20null%3B
The previous implementation of this rule with option "always" even enforced the trailing comma here, which would give you compile errors. I removed the check, because typescript will show errors for unnecessary (invalid) trailing commas in this case.
| @@ -376,8 +375,7 @@ class Test<A, B, C> { | |||
| } | |||
|
|
|||
| foo5< | |||
| T extends Test<[A, A], [A, A,], A>, | |||
| ~ [Unnecessary trailing comma] | |||
There was a problem hiding this comment.
why modify this test case? (other places too)
src/rules/trailingCommaRule.ts
Outdated
| @@ -19,13 +19,18 @@ import * as ts from "typescript"; | |||
|
|
|||
| import * as Lint from "../index"; | |||
|
|
|||
| interface Options { | |||
| multiline: string; | |||
There was a problem hiding this comment.
make this of type "always" | "never"?
| class TrailingCommaWalker extends Lint.AbstractWalker<Options> { | ||
| public walk(sourceFile: ts.SourceFile) { | ||
| const cb = (node: ts.Node): void => { | ||
| switch (node.kind) { |
src/rules/trailingCommaRule.ts
Outdated
| } | ||
| const sourceText = this.sourceFile.text; | ||
| for (const member of members) { | ||
| // PropertSignature in TypeLiteral can end with semicolon or comma. If one ends with a semicolon, don't check for trailing comma |
| this.lintChildNodeWithIndex(node, i + 1); | ||
| } | ||
| }); | ||
| private checkListWithEndToken(node: ts.Node, list: ts.NodeArray<ts.Node>, closeTokenKind: ts.SyntaxKind) { |
There was a problem hiding this comment.
you don't really need closeTokenKind as a parameter since it's always called withCloseParenToken, but fine as-is
There was a problem hiding this comment.
I'd prefer to leave it as is.
Maybe someday type parameters can have trailing commas. Then this method could also handle GreaterThenToken.
|
@ajafff thanks! |
PR checklist
What changes did you make?
[bugfix] No longer enforce trailing commas in type parameters and tuple types. Fixes: #1905
[enhancement] New checks for CallSignature and NamedExports
Removed buggy check for interfaces.
Is there anything you'd like reviewers to focus on?
This removes the buggy check for interfaces (Ref #1810). I could not find any smart logic to find out if trailing comma is desired or not. That maybe needs a new config option. Let me know if this is needed to get this merged.