Implement -IncludeSuppressions parameter

[rjmholt]

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.

[rjmholt]

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...

[rjmholt]

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...

[JamesWTruher]

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.

[rjmholt]

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

[bergmeister]

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

[JamesWTruher]

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

[rjmholt]

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.

[bergmeister]

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 :-)

[rjmholt]

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.

[bergmeister]

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 @JamesWTruher decide on this matter. I am happy with the rest of the PR.

[JamesWTruher]

@rjmholt @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.

[rjmholt]

@bergmeister @JamesWTruher need an approval to merge 🙂