TSLint rules and fixes #3642
TSLint rules and fixes #3642
Conversation
|
I'm trying out this Reviewable tool for code reviews. Still a few kinks to work out but it could be a nice improvement over GitHub's UX. |
| CountMask = 0x0FFFFFFF, // Temp variable counter | ||
| _i = 0x10000000, // Use/preference flag for '_i' | ||
| _i = 0x10000000, // Use/preference flag for '_i' |
There was a problem hiding this comment.
Does TSLint require that these become unaligned?
There was a problem hiding this comment.
I think it depends on the ruleset. I don't have any alignment rules turned on and I'm not sure how they'll work when they conflict with things like whitespace rules (which is presumably the issue here). I'll just preserve the alignment for now since we're so far from 0 TSLint errors.
|
What's the intended process in terms of version updates? New features added to TypeScript (such as intersection types #3622) can easily break TSLint's ability to parse the compiler's code. I wonder if the only way to make it work is to have tslint failures reported in CI somehow. Without that feedback a genuine tslint break (fail to parse) risks to become detached from the commit causing the parsing failure. |
| @@ -5565,12 +5565,12 @@ var __param = (this && this.__param) || function (paramIndex, decorator) { | |||
| // we need to add modules without alias names to the end of the dependencies list | |||
|
|
|||
| let aliasedModuleNames: string[] = []; // names of modules with corresponding parameter in the | |||
| // factory function. | |||
| // factory function. | |||
There was a problem hiding this comment.
i do not think this is better.
There was a problem hiding this comment.
Agreed. Either move the comments above or we'll have to reconsider the rules
There was a problem hiding this comment.
Agree. This kind of comment (required breaking into two line in this manner) should be move above
There was a problem hiding this comment.
Yep, will put it above, strange style to put it afterwards like this
|
@mihailik to be determined. I talked with Ryan briefly yesterday about seeing whether we could have TSLint just use the latest TypeScript parser but that may or may not be feasible. I would certainly like these reported in CI at some point. First we have to agree on rules :) Then clean up, then figure out how to make the workflow work. There're already language changes in our latest bits that are breaking TSLint (ex namespace instead of module triggers a missing semicolon warning). We'll need to figure out the right way to keep things minimally painful. |
|
@danquirk We would love to have a version of TSLint track the develop branch of TypeScript so we're always up to date. It's been on our roadmap to do that. // CC @adidahiya @leeavital @derekcicerone |
|
Roslyn has a built-in support for lint-like rules. Should TypeScript start to move in the same strategic direction? Having 'tslint core' converged into TypeScript services would add value to both projects, and coincidentally ensure tslint stays up to date. Tslint project in that world would contribute the core classes into TS services, and become a repository of rules (similar to DefinitelyTyped, but more managed). |
|
At the very least, TypeScript using TSLint is great because TSLint will be able to benefit from dogfooding on new language constructs. @CyrusNajmabadi might have some ideas with respect to @mihailik's last comment. |
|
👍 We can track TypeScript's develop branch to make sure TSLint is always up to date with the latest language services. |
|
|
||
| // All conflict markers consist of the same character repeated seven times. If it is | ||
| // a <<<<<<< or >>>>>>> marker then it is also followd by a space. | ||
| "const": SyntaxKind.ConstKeyword, |
|
Reviewed 2 of 2 files at r1, 4 of 10 files at r2, 5 of 8 files at r3, 3 of 3 files at r4. src/compiler/emitter.ts, line 14 [r4] (raw file): src/compiler/emitter.ts, line 169 [r4] (raw file): src/compiler/emitter.ts, line 5972 [r4] (raw file): src/compiler/scanner.ts, line 57 [r7] (raw file): Comments from the review on Reviewable.io |
|
Should add a jake task Comments from the review on Reviewable.io |
|
@mhegazy @DanielRosenwasser cleaned it up, removed a few rules, look good? Currently this is just a jake task until TSLint supports using the latest typescript branch at which point we could clean up any remaining errors and start running this task automatically next to runtests. |
| if (body.kind === SyntaxKind.SourceFile || body.kind === SyntaxKind.ModuleBlock) { | ||
| for (let stat of (<Block>body).statements) { | ||
| if (stat.kind === SyntaxKind.ExportDeclaration || stat.kind === SyntaxKind.ExportAssignment) { | ||
| return true; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
might've been the rule getting for...of line, will double check
Edit: yeah, removing it since the rule still isn't satisfied anyway
|
👍 |
|
New PR in #3803 will be the one that's merged |
With the somewhat recent proliferation of human authored style comments in PRs I thought it worth considering setting up TSLint and appropriate rule support. The current rule list is not final and I only made non-controversial fixes (namely, not adding braces to every if statement yet).
This doesn't get us to the point where we can actually fail PRs due to TSLint failures but it at least lets you locally run the checks. TSLint requires a few fixes due to recent language changes.