Implement -IncludeSuppressions parameter by rjmholt · Pull Request #1701 · PowerShell/PSScriptAnalyzer · GitHub

PR Summary

Implements the -IncludeSuppressions parameter, allowing a single Invoke-ScriptAnalyzer invocation to emit all diagnostics including suppressed ones, decorating the suppressed ones accordingly.

Also adds an IsSuppressed property on diagnostics.

/cc t-lipingma

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking
Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

…well

Ok, I've spent ages trying to debug this but it doesn't make any sense. The new parameter sets aren't present, but the command is loaded from the asset location, and this only happens in WinPS 5.1...

…dlet changes

I finally worked this out. We have a bizarre test that overrides Invoke-ScriptAnalyzer and then calls other tests... Naturally now we have to fix all of that. I've never seen anyone actually call into the theoretically exposed API...

Is this a bug with the original code? If I read this correctly we will take the action no matter what, but ShouldProcess is supposed to notify the user and not take the action in the case of -WhatIf. this code will fix the script even with -WhatIf. I get that it's not as big a deal for just the diagnosis part, but the fix part seems wrong.

Yeah I saw this and had the same thought. Definitely something we should review. If Christoph Bergmeister (@bergmeister) agrees, I can open an issue to track it so we can fix it beyond this PR.

This goes back to my very first PR in PSSA, where I added the -Fix switch and since this is an action that changes state, I implemented ShouldProcess for it and declared that on the cmdlet. Since the -Fix parameter is only on the -File parameter set, I did not implement it for the -ScriptDefinition parameter set. Original commit is here: bfa1c54
Therefore I still think it's not really important here, very rarely does one run PSSA where the command would take ages and performing a dry-run would save time. Still happy to have a tracking issue

Could this ever be called with a suppression list count of zero? In that case IsSuppressed really true?

Very good question. I think the mere wrapping in the SuppressedRecord object classes the diagnostic as suppressed, but I agree that this opens up the possibility of an invalid object, which I dislike... I'm not sure I see a way around this without changing a few things around though, and I'm not sure that's worth it.

Overall looks good but not sure whether doubling the parameter sets is the right way. This can make the experience more confusing for users and makes maintenance more complicated as well. How about an Enum parameter instead that has a default?
Some good improvements to tests that made some assumptions :-)

This can make the experience more confusing for users and makes maintenance more complicated as well. How about an Enum parameter instead that has a default?

So I would prefer an enum parameter, but that would be a breaking change; invocations that previously use -SuppressedOnly will no longer work.

In order to keep -SuppressedOnly while also including another parameter that affects suppression output, I think we're forced to split things into parameter sets in some way. The two mutually exclusive switches seemed like a simple approach. An alternative might be putting a more future-proof enum in the other parameter set.

Hmm, didn't realize -SuppressedOnly was an already existing parameter.
But I think we could still have the -IncludeSuppressions switch side by side with it but instead have some early validation logic to disallows the usage of both switches and throw an error (yes, this would be at runtime unfortunately). To me, multiple param sets for different switches not only make the code more complex but are also unintuitive as a user when I try to do tab completion and might not even be aware of other params that are excluded just because I chose to use one param and therefore don't know in most cases that this unintentional action is limiting what tab completion shows. Also, in case one person actually reads the docs and uses both switches, PowerShell's generic error is not helpful to users and although users could look at the cmdlet syntax block and try to understand what the param sets are, the reality is that most users don't understand it or just end up being frustrated by the time it takes them to figure out how to call the command.

Having said that, this topic is opinionated and I am sure the other side will argue for param sets with its early validation before actually running the command. Therefore I suggest we let a 3rd person like James Truher (@JamesWTruher) decide on this matter. I am happy with the rest of the PR.

Rob Holt (@rjmholt) Christoph Bergmeister (@bergmeister) i'm generally in favor of adding parameter sets where they make sense and don't over complicate things. In this case we're going from 2 to only 4 so i don't believe it's a higher cognitive load. The benefit with parameter sets is that we can determine behavior via inspection rather than runtime, so tools like vscode provide a better experience.

Christoph Bergmeister (@bergmeister) James Truher (@JamesWTruher) need an approval to merge 🙂

Rob Holt (rjmholt) added 2 commits July 24, 2021 01:39

Implement IncludeSuppressed switch to emit suppressed diagnostics as …

93a0b02

…well

Add tests

e5c48e5

This comment has been minimized.

Sign in to view

Rob Holt (rjmholt) added 12 commits July 24, 2021 09:48

Fix default parameter set

a2c64a3

Use mandatory to tie break parameter binding

c99cf42

Fix parameter set processing

8fbbaf7

Fix parameter set names

a450a42

Fix parameter set tests

273d0d6

Update help for new parameter set

bcf66f9

Reinstate bad parameter set for -Recurse

643ff4c

Fix help tests to include cmdlet parameters

c50eed5

Fix PSSA build asset path

9548bcd

Wrestle with modules

4ebe0bb

Get module info

d91a6f3

Fix

5465831

Rob Holt (rjmholt) added 2 commits July 24, 2021 15:09

Update internal Invoke-ScriptAnalyzer redefinition to conform with cm…

fdaca95

…dlet changes

Rob Holt (rjmholt) added 3 commits July 25, 2021 10:52

Fix LibraryUsage tests

8d09ac0

Add IsSuppressed property to diagnostics

50bdfa9

Clean up processing logic in Invoke-ScriptAnalyzer

6e9e30c

This comment has been minimized.

Sign in to view

Update README with new parameter syntax

02ccd84

Rob Holt (rjmholt) requested review from James Truher (JamesWTruher) and Christoph Bergmeister (bergmeister) July 26, 2021 14:20

James Truher (JamesWTruher) reviewed Jul 28, 2021

View reviewed changes

Christoph Bergmeister (bergmeister) reviewed Jul 28, 2021

View reviewed changes

Rob Holt (rjmholt) requested review from James Truher (JamesWTruher) and Christoph Bergmeister (bergmeister) August 3, 2021 17:11

Rob Holt (rjmholt) force-pushed the includesuppressions-parameter branch from c970e73 to 02ccd84 Compare August 3, 2021 22:15

Rob Holt (rjmholt) enabled auto-merge (squash) August 4, 2021 15:45

Christoph Bergmeister (bergmeister) approved these changes Aug 4, 2021

View reviewed changes

Rob Holt (rjmholt) merged commit d1942a1 into PowerShell:master Aug 4, 2021

Rob Holt (rjmholt) deleted the includesuppressions-parameter branch November 20, 2021 00:53

-                              WriteToOutput(diagnosticsList);
+                              if (fix)
+                              {
+                                  ShouldProcess(path, $"Analyzing and fixing path with Recurse={this.recurse}");

                       public SuppressedRecord(DiagnosticRecord record, IReadOnlyList<RuleSuppression> suppressions)
                       {
                           Suppression = new ReadOnlyCollection<RuleSuppression>(new List<RuleSuppression>(suppressions));
+                          IsSuppressed = true;

Implement -IncludeSuppressions parameter by rjmholt · Pull Request #1701 · PowerShell/PSScriptAnalyzer · GitHub

Uh oh!

Implement -IncludeSuppressions parameter - #1701

Implement -IncludeSuppressions parameter#1701Rob Holt (rjmholt) merged 20 commits intoPowerShell:masterPowerShell/PSScriptAnalyzer:masterfrom rjmholt:includesuppressions-parameterrjmholt/PSScriptAnalyzer:includesuppressions-parameterCopy head branch name to clipboard

Conversation

Rob Holt (rjmholt) commented Jul 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

This comment has been minimized.

Rob Holt (rjmholt) commented Jul 24, 2021

Uh oh!

Rob Holt (rjmholt) commented Jul 25, 2021

Uh oh!

This comment has been minimized.

James Truher (JamesWTruher) Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

Rob Holt (rjmholt) Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

Christoph Bergmeister (bergmeister) Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

James Truher (JamesWTruher) Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

Rob Holt (rjmholt) Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

Christoph Bergmeister (bergmeister) left a comment

Choose a reason for hiding this comment

Uh oh!

Rob Holt (rjmholt) commented Jul 29, 2021

Uh oh!

Christoph Bergmeister (bergmeister) commented Aug 2, 2021

Uh oh!

James Truher (JamesWTruher) commented Aug 3, 2021

Uh oh!

Rob Holt (rjmholt) commented Aug 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Implement -IncludeSuppressions parameter#1701
Rob Holt (rjmholt) merged 20 commits into
PowerShell:masterfrom
rjmholt:includesuppressions-parameter

Rob Holt (rjmholt) commented Jul 24, 2021 •
edited
Loading