Skip to content

Provide quickfix functionality #304

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

Merged
merged 4 commits into from
Dec 5, 2016
Merged

Provide quickfix functionality #304

merged 4 commits into from
Dec 5, 2016

Conversation

kapilmb
Copy link

@kapilmb kapilmb commented Nov 21, 2016

No description provided.

@msftclas
Copy link

Hi @kapilmb, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Kapil Borle). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

Copy link
Contributor

@rkeithhill rkeithhill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! See the one comment below about the rules. Otherwise, I look forward to seeing this in action!

"PSReservedCmdletChar",
"PSReservedParams",
"PSShouldProcess",
"PSMissingModuleManifestField",
"PSAvoidDefaultValueSwitchParameter",
"PSUseDeclaredVarsMoreThanAssigments",
"PSMisleadingBacktick",
"PSUseDeclaredVarsMoreThanAssigments"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should test the new rule additions. We got a lot of blow-back from the community early on because of lots of green squiggles in folks' scripts. Also, we should add a note in the comments for this section, that any updates here should be also made in the https://github.com/PowerShell/vscode-powershell/blob/master/examples/PSScriptAnalyzerSettings.psd1 file as well.

BTW it is more palatable for folks when the green squiggles underline just the bare minimum of the script as opposed to say entire functions. :-) BTW that also helps with not having "overlapping" script extents for multiple rule violations.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was testing PSUseDeclaredVarsMoreThanAssigments so I added it there and forgot to remove it. The rule is still a bit noisy and I would rather remove it. Thanks for catching it.

@rkeithhill
Copy link
Contributor

@kapilmb Sorry, I wasn't reffering to PSUseDeclaredVarsMoreThanAssigments as that rule has been in there for a while. It was the other rules you added. I'm not saying they need to come out - just that we need to verify they don't cause mass green squiggles. :-) Specifically referring to PSAvoidUsingPlainTextForPassword and PSUseToExportFieldsInManifest. If the green squiggle impact isn't too bad then I say leave them in.

@kapilmb
Copy link
Author

kapilmb commented Nov 21, 2016

@rkeithhill Yes, you are right. PSUseDeclaredVarsMoreThanAssignments has been there for a while.

The extents of PSAvoidUsingPlainTextForPassword and PSUseToExportFieldsInManifest do not cause mass green squiggles. However, the extent of PSAvoidUsingCmdletAliases spills over to the entire command. I will fix the extent in the next PSSA release. So, I guess I will just revert the previous commit and after reverting the rule list will look like this:

            "PSUseToExportFieldsInManifest", # provides quickfix
            "PSMisleadingBacktick", # provides quickfix
            "PSAvoidUsingCmdletAliases", # provides quickfix
            "PSUseApprovedVerbs",
            "PSAvoidUsingPlainTextForPassword", # provides quickfix
            "PSReservedCmdletChar",
            "PSReservedParams",
            "PSShouldProcess",
            "PSMissingModuleManifestField",
            "PSAvoidDefaultValueSwitchParameter",
            "PSUseDeclaredVarsMoreThanAssigments"

One thing I would like to note is that the quickfix scenario will not work until the next release of PSSA (which should be soon.)

@kapilmb kapilmb force-pushed the kapilmb/code-actions branch from 2ab1de4 to 1d2d45e Compare November 22, 2016 01:30
@kapilmb
Copy link
Author

kapilmb commented Nov 22, 2016

@rkeithhill I have reverted the "rule removal" changes.

@rkeithhill
Copy link
Contributor

I didn't notice the addition of PSAvoidUsingCmdletAliases before - sorry. You might want to run that by @daviwil. That rule is likely to light up a lot in scripts. :-)

@kapilmb kapilmb force-pushed the kapilmb/code-actions branch from 1d2d45e to 24a34c1 Compare December 3, 2016 02:22
@kapilmb
Copy link
Author

kapilmb commented Dec 3, 2016

@daviwil rebased on develop branch.

@daviwil daviwil merged commit a2bb4ad into develop Dec 5, 2016
@daviwil daviwil deleted the kapilmb/code-actions branch December 5, 2016 14:25
@daviwil
Copy link
Contributor

daviwil commented Dec 5, 2016

Thanks a lot Kapil!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants