WIP: enable tslint and fix tslint warnings #1148

rkeithhill · 2018-01-04T03:54:16Z

This is definitely a work in progress. Just wanted other folks to be able to see the changes/progress.

This is out of sync with the latest (SaveFile) changes to the ExtensionCommands.ts file. I'll merge an update to that later.

TylerLeonhardt · 2018-01-04T04:31:15Z

Those merge conflicts are ROUGH. We should try to get this PR in sooner rather then later, otherwise the conflicts are going to be worse. 😞

rkeithhill · 2018-01-04T05:35:50Z

No sweat. I was able to rebase onto master - no more merge conflicts - for now.

TylerLeonhardt · 2018-01-04T06:28:36Z

src/controls/animatedStatusBar.ts

@@ -3,14 +3,14 @@
 *--------------------------------------------------------*/

 import {
-    StatusBarItem,
+    Disposable,
    StatusBarAlignment,


Alpha sorting 😍

Yup, that is one of the things that TSLINT is picky about.

it's beautiful 😍

TylerLeonhardt · 2018-01-08T20:32:55Z

.vscode/extensions.json

+{
+  // See http://go.microsoft.com/fwlink/?LinkId=827846
+  // for the documentation about the extensions.json format
+  "recommendations": [


What's this? I'm curious

When this project folder is opened, VS Code will prompt the user to suggest that these extensions are installed when working on this project.

TylerLeonhardt · 2018-01-08T20:34:12Z

src/controls/checkboxQuickPick.ts

        quickPickItems.push({
                label: convertToCheckBox(item),
-                description: item.description
+                description: item.description,


this extra , was always weird to me... but I'm not picky

Trailing comma rules seem to be pretty common in the JavaScript linter world...

for sure - just commenting that it looks weird to me. Especially since in JSON it yells at you if you have a trailing comma.

Don't change because it looks weird to me though :) Let's do what the linters say.

TylerLeonhardt

Overall, looks great, @rkeithhill! I'm guessing you ran tslint auto-fix for most of this and did some misc changes as well?

I'm just curious about a few things so I'm requesting changes - but all in all, it looks really good. And cleaner!

TylerLeonhardt · 2018-01-08T21:14:26Z

src/features/DebugSession.ts

-                        let msg = "To debug '" + currentDocument.fileName +
-                                  "', change the document's language mode to PowerShell or save the file with a PowerShell extension.";
+                    } else {
+                        const msg = "To debug '" + currentDocument.fileName + "', change the document's " +


@nitpick: I'm surprised it didn't yell at you for not changing this into the string interpolation format. `string text ${expression} string text`

I think they usually prefer that syntax.

Yeah, that didn't trigger the TSLINT extension for VSCode. If you haven't installed that extension, go ahead and do that. I've fixed almost all the warning except a few I wasn't sure about - see src/session.ts and src/features/DocumentFormatter.ts. The extension will red squiggle areas where there are warnings.

TylerLeonhardt · 2018-01-08T21:20:36Z

src/features/Examples.ts

@@ -22,9 +22,10 @@ export class ExamplesFeature implements IFeature {
    }

    public setLanguageClient(languageclient: LanguageClient) {
+        // Eliminate tslint warning


Should this throw like.. a "not implemented" exception or something?

I don't think so. A number of IFeature implementations don't do anything with this particular method i.e. they don't need the LanguageClient instance. TSLint doesn't seem to like empty interface methd impl IIRC.

TylerLeonhardt · 2018-01-08T21:31:00Z

src/features/GenerateBugReport.ts


 ## Environment Information ##

 ### Visual Studio Code ###

 | Name | Version |
 | --- | --- |
-| Operating System | ${os.type() + ' ' + os.arch() + ' ' + os.release()} |
+| Operating System | ${os.type() + " " + os.arch() + " " + os.release()} |


Nit: Is this a combination of `${}` and+""+?

Can we just make that ${os.type()} ${os.arch()} ${os.release()}?

That seems reasonable to me. For the sq to dq issue, I invoked the "auto-fix on entire doc" code action.

Yup. I like that better.

TylerLeonhardt · 2018-01-08T21:40:46Z

src/features/OpenInISE.ts

            }

+            ISEPath += "\\WindowsPowerShell\\v1.0\\powershell_ise.exe";
+
            ChildProcess.exec(ISEPath + ' -File "' + uri.fsPath + '"').unref();


Same comment about string interpolation. Also surprised it didn't catch the use of ' ' instead of " "

I think it doesn't apply that rule when there are embedded quotes. String interpolation would be nice here.

+1 for String interpolation 😃

TylerLeonhardt · 2018-01-08T21:41:18Z

src/features/OpenInISE.ts

+
+    public setLanguageClient(languageClient: LanguageClient) {
+        // Not needed for this feature.


same comment about "not implemented" exception

TylerLeonhardt · 2018-01-08T21:44:46Z

src/features/PowerShellFindModule.ts


            // Cancel the loading prompt after 60 seconds
            setTimeout(() => {
                if (this.cancelFindToken) {
                    this.clearCancelFindToken();

                    vscode.window.showErrorMessage(
-                        "The online source for PowerShell modules is not responding. Cancelling Find/Install PowerShell command.");
+                        "The online source for PowerShell modules is not responding. " +


looks like there's a few instances of concatenation instead of interpolation. tslint must not have a check for that. In any case, if it doesn't care, I don't care that much about changing these.

The warning here was for line too long. I think it warns if we run past either 115 or 120 chars. That is probably configurable but some of these line were too long and it is simple enough to fix that for a string. :-)

TylerLeonhardt · 2018-01-08T21:52:31Z

src/platform.ts

                    let exePath = path.join(item, "pwsh.exe");
                    if (!fs.existsSync(exePath)) {
                        exePath = path.join(item, "powershell.exe");
                    }

                    return {
                        versionName: `PowerShell Core ${path.parse(item).base}`,
-                        exePath: exePath
+                        exePath,


I like this - it's so clean :)

TylerLeonhardt · 2018-01-08T21:55:14Z

src/process.ts

-                reject(e);
-            }
-        });
+                    // this.powerShellProcess.stderr.on(


We don't want commented out code, right?

Agreed. But this isn't something the linter warned on. I can remove it or we can remove it in a later PR. Up to you.

Sounds good! I thought that I saw you delete other commented out code for tslint but I guess not :) I'm fine with this being in a later PR.

I was keeping this around for some reason but I think it can safely be deleted now.

OK since we are doing general warning clean up anyway, I'll go ahead and remove this.

TylerLeonhardt · 2018-01-08T21:57:48Z

src/session.ts


            if (this.inDevelopmentMode) {
-                var devBundledModulesPath =
+                const devBundledModulesPath =
                    // this.sessionSettings.developer.bundledModulesPath ||


no commented out code, right?

Same reply as above.

I saw this line commented out over the weekend and wasn't sure why I did it... I think we should probably uncomment that otherwise the setting is useless.

Does this fire any neurons - 5f9cfd6

Yes, thanks for that! The setting is being used here so you can safely delete that commented out line

TylerLeonhardt · 2018-01-08T21:59:25Z

src/session.ts

                    this.log.write(
-                        `\nWARNING: In development mode but PowerShellEditorServices dev module path cannot be found (or has not been built yet): ${devBundledModulesPath}\n`);
+                        `\nWARNING: In development mode but PowerShellEditorServices dev module path cannot be " +


Another weird mix of concatenation and interpolation. Does this need to be addressed?

Yup, another case of "line too long". I'm open to alternate ways to fix this but when you have a long message, you just need to break it up. :-)

Or we could disable this particular rule. However, in genera I think it is a good rule to have.

Yeah this one in particular is weird to me because it starts off with ` but ends in "

it just doesn't seem... correct? I guess it still works?

I'm surprised the linter didn't catch that. I'll check it tonight.

Wow, I'm surprised neither TypeScript nor the linter complained about that. Fixed.

TylerLeonhardt · 2018-01-08T22:14:23Z

@rkeithhill you're awesome for doing this! I'd love to get what you have here in 1.6.0 if we can 😃

rkeithhill · 2018-01-08T22:57:08Z

I'm guessing you ran tslint auto-fix for most of this and did some misc changes as well?

Yes. The only manual changes were the export namespace <MessageName>Type { ... } changes to export const type = .....

daviwil

The only thing I'm not crazy about is the I prefix on interface names. I know that we generally do that in other languages but I've seen a lot of TS code without it. Main thing I'd be concerned about is any new bugs introduced with the message types that have changed, but some cursory manual testing should verify whether things are fine.

daviwil · 2018-01-08T22:59:03Z

src/process.ts

-                reject(e);
-            }
-        });
+                    // this.powerShellProcess.stderr.on(


I was keeping this around for some reason but I think it can safely be deleted now.

TylerLeonhardt · 2018-01-09T02:21:01Z

Try to test a bunch of scenarios with this one if you can since it's such a big change. Let me know if you need anything from me.

rkeithhill · 2018-01-09T02:21:47Z

Should I use GitHub's Squash and merge or would it be better to do a rebase -i or should we do a merge commit to preserve the individual commits? This is of course, after doing some more testing on this branch.

TylerLeonhardt · 2018-01-09T16:45:17Z

Squash and merge is fine IMO.

daviwil · 2018-01-09T17:31:48Z

Yep, squashing is fine.

rkeithhill · 2018-01-18T04:47:26Z

I've tested all the PowerShell commands and everything seems to be working. There are two possibly legit issues that I need someone else to look at. One is in session.ts in the resolveCodeLens method where there is some potential variable shadowing going on. The other is in the sendDocumentFormatRequest method in DocumentFormatter.ts.

So, if there are no objections I propose we merge this.

TylerLeonhardt · 2018-01-18T17:19:44Z

@rkeithhill
RE: resolveCodeLens.... are you talking about the assignment of codeLens.command.arguments? That DOES seem a bit fishy. We could probably do one of these guys to be safe:

const oldArgs = Object.assign([], codeLens.command.arguments)`

or... I heard that new spread operator copies arrays too.

[...oldArgs] = codeLens.command.arguments

daviwil · 2018-01-18T17:24:03Z

Is there a linting error on that code? I think it is fine as is, and unfortunately necessary due to a problem I ran into with Code Lens.

rkeithhill · 2018-01-18T17:27:29Z

Here is the warning:

The codeLens in the interior function assigned to resolveFunc takes a codeLens variable that is the same name as the exterior function's parameter. If this has to be that way, let me know and I can suppress the warning.

daviwil · 2018-01-19T14:45:11Z

Ahhh shit, sorry, I missed that detail :) Yeah I'd say definitely rename the inner parameter name to something like codeLensToFix just in case. Thanks a bunch for doing this! Now I think I want to set up TSLint on ide-powershell :)

rkeithhill · 2018-01-19T17:53:53Z

Ok, codeLensToFix change has been committed. That leaves just the DocumentFormatter issue. I think I'll merge this tomorrow morning. Getting ready to walk over with the kids to Disneyland for the day. :-)

daviwil · 2018-01-19T21:04:17Z

Dude, you shouldn't be committing code, go have some fun! 😄

rkeithhill · 2018-01-20T19:23:29Z

Merging this. We'll need to fix the DocumentFormatter issue in another PR and also turn on linting during TS compilation.

rkeithhill · 2018-01-20T19:26:24Z

@tylerl0706 Could you do a pull and try running master on your Mac to see if you encounter any issues.

TylerLeonhardt · 2018-01-21T20:01:24Z

Can do!

rkeithhill requested review from daviwil and TylerLeonhardt January 4, 2018 03:54

rkeithhill added 4 commits January 3, 2018 22:31

Enable tslint - clean up main.ts

d7db831

Clean up tslint warings in rest of top-level ts files

d0cc8c6

WIP - eliminate more tslint warnings

381cbc1

Fix more tslint warnings

58fca4c

rkeithhill force-pushed the rkeithhill/enable-tslint branch from 2bece0e to 58fca4c Compare January 4, 2018 05:34

TylerLeonhardt reviewed Jan 4, 2018

View reviewed changes

rkeithhill added 4 commits January 4, 2018 00:13

More TSLINT cleanup

a54f6ab

More TSLINT warning cleanup

ea808b5

Close to final on TSLINT cleanup

e1acbb1

Add recommended extensions.json

b36e21d

TylerLeonhardt reviewed Jan 8, 2018

View reviewed changes

TylerLeonhardt suggested changes Jan 8, 2018

View reviewed changes

daviwil approved these changes Jan 8, 2018

View reviewed changes

Address PR feedback, manually tested

634ccb2

TylerLeonhardt approved these changes Jan 9, 2018

View reviewed changes

Fix tslint warning on shadowed var name

7ff52f4

rkeithhill merged commit 8d8f2d6 into master Jan 20, 2018

rkeithhill deleted the rkeithhill/enable-tslint branch January 20, 2018 19:23

rkeithhill mentioned this pull request Jan 23, 2018

address tslint issues #1086

Closed


		public setLanguageClient(languageClient: LanguageClient) {
		// Not needed for this feature.

WIP: enable tslint and fix tslint warnings #1148

WIP: enable tslint and fix tslint warnings #1148

Conversation

rkeithhill commented Jan 4, 2018

TylerLeonhardt commented Jan 4, 2018

rkeithhill commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt commented Jan 8, 2018 • edited Loading

rkeithhill commented Jan 8, 2018

daviwil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt commented Jan 9, 2018

rkeithhill commented Jan 9, 2018 • edited Loading

TylerLeonhardt commented Jan 9, 2018

daviwil commented Jan 9, 2018

rkeithhill commented Jan 18, 2018

TylerLeonhardt commented Jan 18, 2018

daviwil commented Jan 18, 2018 • edited Loading

rkeithhill commented Jan 18, 2018

daviwil commented Jan 19, 2018

rkeithhill commented Jan 19, 2018

daviwil commented Jan 19, 2018

rkeithhill commented Jan 20, 2018

rkeithhill commented Jan 20, 2018

TylerLeonhardt commented Jan 21, 2018

TylerLeonhardt left a comment •

edited

Loading

TylerLeonhardt Jan 8, 2018 •

edited

Loading

TylerLeonhardt commented Jan 8, 2018 •

edited

Loading

rkeithhill commented Jan 9, 2018 •

edited

Loading

daviwil commented Jan 18, 2018 •

edited

Loading