Script analysis should default to a minimal set of rules which can be configured

[Jaykul]

It's wrong far more often than it's right on my code, and the annoyance of the warnings is really starting to get to me.

[kilasuit]

Theres really a few options here

Dont use VSCode with the PowerShell extension
Disable PSScriptAnalyzer when using PowerShell extension with VSCode as suggested by @daviwil in this tweet https://twitter.com/daviwil/status/669328381306015745 (included for further reference to others reading this issue)
Add feedback to PSScriptAnalyzer to enhance it with some repeatable code samples that others could try (I know you are already doing this)

We shouldnt really be suggesting to take a step back in terms of getting best (better/good - all debatable) practices embaked in the development experience as this will only serve to further discourage more adoption.

Or the other possibility (and I kid when I say this) write better code.

We are getting a better overall ecosystem but it will much more time to do so to the point where there is a majority in agreement about rules etc but at least we have a beginning framework

Just my 2 pennies worth

[daviwil]

In the 0.2.0 release I added a new setting (see #11) to turn off script analysis entirely. Instead of disabling it by default, I'd like to work out a good set of default rules that will be helpful to people but not overly noisy or wrong/misleading.

Part of the reason for introducing the powershell.scriptAnalysis settings namespace is to set the stage for more analysis settings to be added. One in particular that I have in mind is a 'profile' setting which would target PSScriptAnalyzer's new profile feature. This would allow the user to customize the ruleset to their preferences.

What do you think?

[daviwil]

Also, to reinforce one of @kilasuit's points, at some point soon I'm going to try to tune some of the Script Analyzer rules so that they don't mark whole functions unnecessarily. Only the most minimally relevant section of the script should be marked for any diagnostic that is returned.

[Jaykul]

So, I will admit I was being provocative here. (Which is why I tweeted it, rather than just filling it)

I would love to have PSScriptAnalyzer be like visual studio code analysis, where every time it yells at me I correct my code, or feel a sense of shame when I disable the rule...

Currently, that's just not the case. My argument to the team in charge of PSScriptAnalyzer has always been that the default rules need to be important, and always right.

But on top of that, it needs to be like Visual Studio code analysis.

• it needs to understand the whole project, not individual files
• it needs to ship rule sets (e.g.: Minimal, Recommended, All, etc)
• solutions, projects (and modules) need to be able to specify the rule set so that new collaborators don't have to do anything to use the same settings I do.

That last bit is where you come in. However, having said all that...

Is there any other language or dev tool that ships with code analysis on? Visual Studio definitely doesn't turn it on by default in my C# projects. Why would you turn it on by default?

[kilasuit]

So my argument here is that having it on by default includes the following points
• Reduces the level of wasted time with for those working with the language that are new to it or even those like yourself that are more used to it
• It negates the use of any poor practises and as you pointed out kind of makes you want to write better code
• As its community based we can all add into the rules & RuleSets therefore being able to add future enhancements to make it run functionally better

[daviwil]

I'm pretty sure Visual Studio has code analysis on for C# by default, otherwise you wouldn't see warnings about uninitialized variables, etc (which I do). But I agree, we need to default to a standard, minimal ruleset which can be configured on a per-project basis. I'll work on this for the next release!

[rkeithhill]

@Jaykul is right. There's a difference between CS (compiler) and CA (code analysis) warnings and errors. I've always had to explicitly turn on code analysis in my C# projects.

[daviwil]

Didn't realize there was a separate code analysis setting in VS. My intent with having analysis on by default is to get that basic level language intelligence which can't be given by the parser alone. Obviously that's now the way it is now, but ultimately I'd like to be able to give some level of helpful hints by default.

How about this: before I make any changes, I'll send out a list of my proposed default rules and you guys can tell me whether you think they make sense.

[rkeithhill]

A "minimum" rule set or profile from the PSScriptAnalyzer team would be a good thing to have. It is useful to see those things that have a high probability of causing failure. However whether I use Where in my script instead of Where-Object is not one of those IMO.

That's the sort of the problem with tools like this, the rules aren't always so cut and dry. Heck the "community" has had a hard time agreeing on what the rules should be. Check out the issues list on the PSScriptAnalyzer project. :-)

[Jaykul]

The default is not to Enable Code Analysis on Build. Of course, if you have ReSharper or CodeRush, or some of these other extensions, they do their own Code Analysis regardless.

[daviwil]

I'm about to make this change and wanted to double-check with folks on what a good basis would be for now (until the new Script Analyzer rule sets get rolled out). I've seen comments from @Jaykul and @rkeithhill on the PSScriptAnalyzer repo about which things they think shouldn't be there by default. Here is the list of rules I intend to include at this point:

PSUseApprovedVerbs
PSReservedCmdletChar
PSReservedParams
PSShouldProcess
PSUseShouldProcessForStateChangingFunctions
PSUseSingularNouns
PSMissingModuleManifestField
PSAvoidDefaultValueSwitchParameter

What do you think? Anything I should add or remove?

[daviwil]

Check out the pull request here:

PowerShell/PowerShellEditorServices#70

[rkeithhill]

I would consider adding:

PSUseDeclaredVarsMoreThanAssigments

[Jaykul]

I'm don't like PSUseShouldProcessForStateChangingFunctions but I can live with it -- As far as I can tell, it just looks at verbs and wants you to use ShouldProcess based on the verb. If the decision was that simple, we wouldn't need the ShouldProcess attribute. 😦

Does PSUseSingularNouns work, or does it just flag things that end in "s" or "es" or something? ;)

[daviwil]

I think I've seen a bug on PSScriptAnalyzer recently stating that PSUseSingularNouns falsely identifies strings at times. I'll take that one out since not everyone will care about that rule.

For PSUseShouldProcessForStateChangingFunctions, it could be helpful to someone so I'll leave it in. I've already started on a PR for PSScriptAnalyzer that will make function-targetting rules like this not mark the entire function but instead just mark the function's name. This should make the appearance of this rule a lot less annoying. What do you think?

[Jaykul]

I think a rule that warns you when you're writing a cmdlet that does something irreversible and tells you to put ShouldProcess on it is a great feature.

I think a rule that says "since you're using 'Remove' you should use ShouldProcess" is dumb. 😧

[daviwil]

Ok, I'll take it out for now until that rule gets improved.

[daviwil]

Since the primary work for this was completed some time ago I'll go ahead and close this issue. Keith has created a new issue (#122) to track the requirement for allowing the user to configure their desired PSScriptAnalyzer ruleset.