Skip to content
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

Restore default PSScriptAnalyzer ruleset #246

Closed
alejandro5042 opened this issue Aug 18, 2016 · 7 comments
Closed

Restore default PSScriptAnalyzer ruleset #246

alejandro5042 opened this issue Aug 18, 2016 · 7 comments
Labels
Issue-Bug A bug to squash.
Milestone

Comments

@alejandro5042
Copy link

I want to use aliases, but as of today's update, I am getting underlines all over my source. They are very distracting and I cannot find a way to disable them. Is there an option? If not, I propose this being an issue that should be fixed.

@daviwil
Copy link
Contributor

daviwil commented Aug 18, 2016

Apologies for that, we have changed to a different method for integration PowerShell Script Analyzer and didn't carry over the default ruleset we were using before. I'll address this in a patch release in a few days.

@daviwil daviwil changed the title I want to use aliases; provide option to ignore the squiggles Restore default PSScriptAnalyzer ruleset Aug 18, 2016
@daviwil daviwil added this to the 0.7.1 milestone Aug 18, 2016
@daviwil daviwil added Issue-Enhancement A feature request (enhancement). Issue-Bug A bug to squash. and removed Issue-Enhancement A feature request (enhancement). labels Aug 18, 2016
daviwil added a commit to PowerShell/PowerShellEditorServices that referenced this issue Aug 23, 2016
This change restores the default set of rules that we previously used in
our PSScriptAnalyzer integration.  This list got removed inadvertently
when we changed our design to consume PSScriptAnalyzer as a module rather
than a C# API.

Related to PowerShell/vscode-powershell#246
daviwil added a commit to PowerShell/PowerShellEditorServices that referenced this issue Aug 23, 2016
This change restores the default set of rules that we previously used in
our PSScriptAnalyzer integration.  This list got removed inadvertently
when we changed our design to consume PSScriptAnalyzer as a module rather
than a C# API.

Related to PowerShell/vscode-powershell#246
@daviwil daviwil mentioned this issue Aug 24, 2016
@daviwil
Copy link
Contributor

daviwil commented Aug 24, 2016

Fixed in the latest release!

@kilasuit
Copy link
Contributor

@daviwil personally i dont feel this is "fixed" as realistically the default ruleset should be to have any rules that emit warnings or errors throw - exactly as it would be if you just ran Invoke-ScriptAnalyzer -Severity Error,Warning

We then can allow users like @alejandro5042 to disable the Aliases rule using their own rulesets but for the greater good of the community and those new to PowerShell they need to be warned about the fact that aliases can break scripts.

For example I regularly see organisations deploy as part of their base OS images a $PROFILE.AllUsersAllHosts that includes

Remove-Item Alias:\* -force

would break all of @alejandro5042 scripts and is exactly why you should never use aliases.

Happy to raise this as another issue but I feel that pleasing 1 user in this case @alejandro5042 isn't in the best interests of the community especially now that it is going to start growing even bigger.

@alejandro5042
Copy link
Author

I know I have already voiced my opinion on the other thread. I'm afraid the use of aliases is "religious" in nature and we'll only have to agree to disagree.

Remove-Item Alias:\* -force

Why should I--an unrelated user--have to support the workflow of someone who is intentionally removing all their aliases? Aliases are part of the PowerShell language and are extremely useful for day-to-day usage. I shouldn't have to pay the penalty because you don't like a built-in language feature.

For example, take this very specific, very simple command, and see how using aliases blows it up into something that's unapproachable and difficult to read.

$buildDefinitions | where TriggerType -match "GatedCheckIn" | foreach Name | sls "^R" | sort Length -desc

vs.

$buildDefinitions | Where-Object TriggerType -match "GatedCheckIn" | Foreach-Object Name | Select-String "^R" | Sort-Object Length -Descending

Now I understand, I'm primarily a C# coder and in C# we spell everything out. But I use PowerShell as a scripting environment where things should be easy and fast.

Again, we can agree to disagree.

@rkeithhill
Copy link
Contributor

IMO the extension shouldn't be super opinionated about this argument but it should make it easy for folks to get what they want. Today, it's perhaps not as obvious as I'd like but you can create your own ruleset file and apply it globally to VSCode on your machine or to a specific workspace by adding and configuring the setting in your user settings file (or workspace settings file for the project specific ruleset - my favorite approach):

// Specifies the path to a PowerShell Script Analyzer settings file. Use either an absolute path (to override the default settings for all projects) or use a path relative to your workspace.
"powershell.scriptAnalysis.settingsPath": "",

The ruleset file is pretty simple:

# The PowerShell Script Analyzer will generate a warning
# diagnostic record for this file due to a bug -
# https://github.com/PowerShell/PSScriptAnalyzer/issues/472
@{
    # Only diagnostic records of the specified severity will be generated.
    # Uncomment the following line if you only want Errors and Warnings but
    # not Information diagnostic records.
    #Severity = @('Error','Warning')

    # Analyze **only** the following rules. Use IncludeRules when you want
    # to invoke only a small subset of the defualt rules.
    IncludeRules = @('PSAvoidDefaultValueSwitchParameter',
                     'PSMissingModuleManifestField',
                     'PSReservedCmdletChar',
                     'PSReservedParams',
                     'PSShouldProcess',
                     'PSUseApprovedVerbs',
                     'PSUseDeclaredVarsMoreThanAssigments')

    # Do not analyze the following rules. Use ExcludeRules when you have
    # commented out the IncludeRules settings above and want to include all
    # the default rules except for those you exclude below.
    # Note: if a rule is in both IncludeRules and ExcludeRules, the rule
    # will be excluded.
    #ExcludeRules = @('PSAvoidUsingWriteHost')
}

The deal with toning down the PSSA errors/warnings early on was due to A) PSSA was highlighting way too much source. For instance, an unapproved verb caused the whole function definition to be squiggled. PSSA has fixed a number of those "extent" issues. B) PS on Linux had not been announced and the issue with aliases was not as, um, critical as it appears to be at the moment.

All that said, I'm not opposed to adding back the avoid aliases rule to the default ruleset. We have various settings to adjust the rules or to simply turn off PSSA. Although it might be worth waiting to see how the RFC on weak aliases works out. Just so we are not ping-ponging around with the default ruleset.

@alejandro5042
Copy link
Author

I agree with waiting to see how the issue settles with PowerShell proper. Naturally, the editor should support whatever default the community decides--but make it easy to override.

@daviwil
Copy link
Contributor

daviwil commented Aug 26, 2016

@rkeithhill is correct, the main reason for cutting down the list of rules to a small set is that there was a lot of marker noise showing up in the average PowerShell file. However, I do think it'd be great if you want to start a separate issue on this repo to get some community consensus about what that default ruleset should be. I am happy to change it to whatever people think is the right default set of rules for the editing experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug A bug to squash.
Projects
None yet
Development

No branches or pull requests

4 participants