-
Notifications
You must be signed in to change notification settings - Fork 519
WIP: enable tslint and fix tslint warnings #1148
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
Conversation
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. 😞
2bece0e to
58fca4c
Compare
No sweat. I was able to rebase onto master - no more merge conflicts - for 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.
Alpha sorting 😍
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.
Yup, that is one of the things that TSLINT is picky about.
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.
it's beautiful 😍
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.
What's this? I'm curious
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.
When this project folder is opened, VS Code will prompt the user to suggest that these extensions are installed when working on this project.
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.
this extra , was always weird to me... but I'm not picky
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.
Trailing comma rules seem to be pretty common in the JavaScript linter world...
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.
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.
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.
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!
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.
@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.
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.
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.
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.
Should this throw like.. a "not implemented" exception or something?
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 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.
src/features/GenerateBugReport.ts
Outdated
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.
Nit: Is this a combination of `${}` and+""+?
Can we just make that ${os.type()} ${os.arch()} ${os.release()}?
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.
That seems reasonable to me. For the sq to dq issue, I invoked the "auto-fix on entire doc" code action.
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.
Yup. I like that better.
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.
Done
src/features/OpenInISE.ts
Outdated
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.
Same comment about string interpolation. Also surprised it didn't catch the use of ' ' instead of " "
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 think it doesn't apply that rule when there are embedded quotes. String interpolation would be nice here.
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.
+1 for String interpolation 😃
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.
Done
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.
same comment about "not implemented" exception
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.
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.
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.
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. :-)
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 like this - it's so clean :)
src/process.ts
Outdated
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.
We don't want commented out code, right?
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.
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.
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.
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.
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 was keeping this around for some reason but I think it can safely be deleted 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.
OK since we are doing general warning clean up anyway, I'll go ahead and remove this.
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.
Done
src/session.ts
Outdated
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.
no commented out code, right?
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.
Same reply as above.
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 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.
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.
Does this fire any neurons - 5f9cfd6
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.
Yes, thanks for that! The setting is being used here so you can safely delete that commented out line
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.
Done
src/session.ts
Outdated
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.
Another weird mix of concatenation and interpolation. Does this need to be addressed?
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.
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. :-)
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.
Or we could disable this particular rule. However, in genera I think it is a good rule to have.
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.
Yeah this one in particular is weird to me because it starts off with ` but ends in "
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.
it just doesn't seem... correct? I guess it still works?
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'm surprised the linter didn't catch that. I'll check it tonight.
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.
Wow, I'm surprised neither TypeScript nor the linter complained about that. Fixed.
@rkeithhill you're awesome for doing this! I'd love to get what you have here in 1.6.0 if we can 😃
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 = .....
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.
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.
src/process.ts
Outdated
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 was keeping this around for some reason but I think it can safely be deleted now.
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.
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.
Squash and merge is fine IMO.
Yep, squashing is fine.
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.
@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
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.
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.
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 :)
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. :-)
Dude, you shouldn't be committing code, go have some fun! 😄
Merging this. We'll need to fix the DocumentFormatter issue in another PR and also turn on linting during TS compilation.
@tylerl0706 Could you do a pull and try running master on your Mac to see if you encounter any issues.
Can do!
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.