Fix latest tslint errors by saschanaz · Pull Request #13006 · microsoft/TypeScript

saschanaz · 2016-12-18T06:51:10Z

Except this type of errors:

export function getNavigationTree(sourceFile: SourceFile): NavigationTree {
    curSourceFile = sourceFile;
    const result = convertToTree(rootNavigationBarNode(sourceFile));
    curSourceFile = undefined;
    return result;
}
let curSourceFile: SourceFile;
// Identifier 'curSourceFile' is never reassigned; use 'const' instead of 'let'.

let response: protocol.Response;
try {
    ({ response } = this.executeCommand(msg));
}
//  Identifier 'response' is never reassigned; use 'const' instead of 'let'.

I think they are tslint bugs rather than actual code problem. (palantir/tslint#1898)

BTW I feel fairly painful to create a new constant for every for-loop, I hope there is a better way to keep high performance...

saschanaz · 2016-12-18T13:34:05Z

A benchmark shows for-of loop is much slower on Chrome (and thus node.js), does this mean we shouldn't use for-of?

ghost · 2016-12-18T19:23:17Z

for-of gets emitted as a for loop, since we use an ES5 target. (So maybe some of those manually-written for loops could be upgraded.)
Also, this would be a good time to delete our own preferConstRule.ts since tslint has that now.

saschanaz · 2016-12-19T04:09:29Z

Already done the upgrade for obvious cases but many of them are using indices so I couldn't do it for them.

… on #13006 to upgrade.

mhegazy

I would inline the collection.length check instead of adding a new variable for it. There is not much perf difference on modern engines anyways.

mhegazy · 2016-12-19T17:34:50Z

            const result: Signature[] = [];
-            for (let i = 0, len = symbol.declarations.length; i < len; i++) {
+            const len = symbol.declarations.length;
+            for (let i = 0; i < len; i++) {


switch to for..of?

Inner lines of code use i for some purpose. It seems I can still switch to for-of in this case but should I do it?

mhegazy · 2016-12-19T17:35:33Z

-                for (let i = isJSConstructSignature ? 1 : 0, n = declaration.parameters.length; i < n; i++) {
+                const n = declaration.parameters.length;
+                for (let i = isJSConstructSignature ? 1 : 0; i < n; i++) {
                    const param = declaration.parameters[i];


for (let i = isJSConstructSignature ? 1 : 0; i < declaration.parameters.length; i++) {

mhegazy · 2016-12-19T18:36:27Z

@saschanaz i have merged a change to move tslint back to 4.0.0-dev.3 (#13028). can you update it with your PR to next again.

… lintfix

saschanaz · 2016-12-19T18:50:16Z

It still won't pass because of some bugs, which should be fixed on tslint side.

mhegazy · 2016-12-19T19:10:29Z

It still won't pass because of some bugs, which should be fixed on tslint side.

do you have a list of the issues?

ghost · 2016-12-19T19:12:04Z

@mhegazy See first post and palantir/tslint#1898. I actually fixed one of them yesterday. The other one is also a legitimate issue, but could be fixed on our end by moving the declaration ( let curSourceFile: SourceFile;) to before the function.

saschanaz · 2016-12-19T19:14:28Z

@Andy-MS I'm still seeing destructuring issue on Travis, should I do something to get the merged fix?

ghost · 2016-12-19T19:17:53Z

tslint hasn't published a new version yet, so there's not much to do but wait.

vladima · 2016-12-19T20:01:14Z

I'd prefer not to change our code to workaround bugs in the toolchain hence palantir/tslint#1908

mhegazy · 2016-12-21T20:54:33Z

@nchen63 any news about palantir/tslint#1908?

nchen63 · 2016-12-23T07:09:05Z

it's merged and included in a new release, v4.2.0

saschanaz · 2016-12-23T15:46:33Z

It seems tslint@next is still giving tslint@4.1.0-dev.0.

nchen63 · 2016-12-23T16:30:33Z

should be fixed now

saschanaz added 3 commits December 10, 2016 17:53

Merge remote-tracking branch 'refs/remotes/Microsoft/master'

3202d59

Merge branch 'master' of https://github.com/Microsoft/TypeScript

603ba89

fix linting errors

27a60e4

msftclas added the cla-already-signed label Dec 18, 2016

This was referenced Dec 18, 2016

Format false branch of ternary operation #5905

Closed

tslint@next does not recognize some reassignation palantir/tslint#1898

Closed

remove preferConstRule.ts

27ed5b8

HerringtonDarkholme mentioned this pull request Dec 19, 2016

Fix #1809, introduce non primitive object type #12501

Merged

saschanaz closed this Dec 19, 2016

saschanaz reopened this Dec 19, 2016

msftclas added the cla-already-signed label Dec 19, 2016

ghost pushed a commit that referenced this pull request Dec 19, 2016

Fix tslint version. 4.1.0-dev.0 introduced some bugs, so we must wait…

ef14c08

… on #13006 to upgrade.

ghost mentioned this pull request Dec 19, 2016

Fix tslint version. 4.1.0-dev.0 introduced some bugs, so we must wait on #13006 to upgrade. #13025

Closed

ghost pushed a commit that referenced this pull request Dec 19, 2016

Fix tslint version. 4.1.0-dev.0 introduced some bugs, so we must wait…

6a578f8

… on #13006 to upgrade.

mhegazy reviewed Dec 19, 2016

View reviewed changes

saschanaz added 2 commits December 20, 2016 03:12

inline length

2a941a7

needless newline

1f79741

mhegazy approved these changes Dec 19, 2016

View reviewed changes

saschanaz added 2 commits December 20, 2016 03:46

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

1a6e43d

… lintfix

next again

67e1fd8

saschanaz closed this Dec 23, 2016

saschanaz reopened this Dec 23, 2016

msftclas added the cla-already-signed label Dec 23, 2016

saschanaz closed this Dec 23, 2016

saschanaz reopened this Dec 23, 2016

msftclas added the cla-already-signed label Dec 23, 2016

saschanaz closed this Dec 23, 2016

saschanaz reopened this Dec 23, 2016

msftclas added the cla-already-signed label Dec 23, 2016

saschanaz closed this Dec 23, 2016

saschanaz reopened this Dec 23, 2016

msftclas added the cla-already-signed label Dec 23, 2016

mhegazy merged commit e4b81d0 into microsoft:master Dec 26, 2016

microsoft locked and limited conversation to collaborators Jun 19, 2018

Conversation

saschanaz commented Dec 18, 2016 • edited by DanielRosenwasser Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saschanaz commented Dec 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Dec 18, 2016

Uh oh!

saschanaz commented Dec 19, 2016

Uh oh!

mhegazy left a comment

Choose a reason for hiding this comment

Uh oh!

mhegazy Dec 19, 2016

Choose a reason for hiding this comment

Uh oh!

saschanaz Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy Dec 19, 2016

Choose a reason for hiding this comment

Uh oh!

mhegazy commented Dec 19, 2016

Uh oh!

saschanaz commented Dec 19, 2016

Uh oh!

mhegazy commented Dec 19, 2016

Uh oh!

ghost commented Dec 19, 2016 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saschanaz commented Dec 19, 2016

Uh oh!

ghost commented Dec 19, 2016

Uh oh!

vladima commented Dec 19, 2016

Uh oh!

mhegazy commented Dec 21, 2016

Uh oh!

nchen63 commented Dec 23, 2016

Uh oh!

saschanaz commented Dec 23, 2016

Uh oh!

nchen63 commented Dec 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

saschanaz commented Dec 18, 2016 •

edited by DanielRosenwasser

Loading

saschanaz commented Dec 18, 2016 •

edited

Loading

saschanaz Dec 19, 2016 •

edited

Loading

ghost commented Dec 19, 2016 •

edited by ghost

Loading