Add some necessary contents for diagnostic suppressions.#1694
Add some necessary contents for diagnostic suppressions.#1694t-lipingma wants to merge 2 commits intoPowerShell:masterfrom
Conversation
|
tests issue needs to solve before merge: #1695 |
1e8aa16 to
2d5c96c
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
2d5c96c to
3139f05
Compare
| /// <summary> | ||
| /// ShowAll: Show the suppressed and non-suppressed message | ||
| /// </summary> | ||
| [Parameter(Mandatory = false)] |
There was a problem hiding this comment.
This isn't mutually exclusive with SuppressedOnly and probably should be. What should a user expect when both are provided?
Given that -SuppressedOnly already exists and we don't want to remove or otherwise break it, we probably can't move to an enum parameter like -SuppressionPreference <Exclude | Include | SuppressedOnly>.
An alternative might be to put each switch into its own parameter set, but that starts to get complicated. We should discuss this further.
There was a problem hiding this comment.
| [Parameter(Mandatory = false)] | |
| [Parameter] |
Mandatory defaults to false
There was a problem hiding this comment.
correct, if it's not mandatory, that should just be omitted. if SuppressedOnly should not be used with this, then parameter sets should be used (and it does get complicated). Although it doesn't look to complicated at the moment it would mean an additional 2 parameter sets.
There was a problem hiding this comment.
yeah. I totally agree with you guys on that point.
If you decided to change the parameter set, let's discuss more details on the design, I'd like to help.
There was a problem hiding this comment.
From our offline conversation:
If the parameter set change is too complicated, can we check in the current solution firstly?
Yeah I think it's a good idea for us to separate out the suppression combining parts from the new parameter/feature. We can then discuss offline or over a call what the best way forward is.
There was a problem hiding this comment.
To maintain PSSA's existing design, I think we can make '-SuppressedOnly' / '-IncludeSuppressions' exclusive in the short term. What do you think about it? Look forward to your reply.
There was a problem hiding this comment.
Btw, ExcludeRule and IncludeRule should also be mutually exclusive.
There was a problem hiding this comment.
make '-SuppressedOnly' / '-IncludeSuppressions' exclusive
Yeah, they should be in different parameter sets.
Btw, ExcludeRule and IncludeRule should also be mutually exclusive.
No, those can be used together to include a non-default rule while excluding a default one.
There was a problem hiding this comment.
To make -SuppressedOnly / -IncludeSuppressions exclusive.
I used the logic below:
Function TestParamSet {
[CmdletBinding(DefaultParameterSetName="File1")]
Param
(
[Parameter(Position = 0,ParameterSetName = 'File1',Mandatory = true)]
[Parameter(Position = 0,ParameterSetName = 'File2',Mandatory = true]
[switch] $Path,
[Parameter(Position = 0,ParameterSetName = 'ScriptDefinition1',Mandatory = true)]
[Parameter(Position = 0,ParameterSetName = 'ScriptDefinition2',Mandatory = true)]
[switch] $ScriptDefinition,
[Parameter(ParameterSetName = 'File1', Mandatory = false)]
[Parameter(ParameterSetName = 'ScriptDefinition1', Mandatory = false)]
[switch] $SuppressedOnly,
[Parameter(ParameterSetName = 'File2', Mandatory = true)]
[Parameter(ParameterSetName = 'ScriptDefinition2', Mandatory = true)]
[switch] $IncludeSuppressions
[Parameter(Position = 0,ParameterSetName = 'File1',Mandatory = false)]
[Parameter(Position = 0,ParameterSetName = 'File2',Mandatory = false]
[switch] $Fix,
)
Process {
#Do Nothing
}
}
so it's to say:
1、Invoke-ScriptAnalyzer C:\build.ps1 --> ParameterSetName==DefaultParameterSetName.
2、Invoke-ScriptAnalyzer -Path C:\build.ps1 --> ParameterSetName==DefaultParameterSetName.
3、Invoke-ScriptAnalyzer -Path C:\build.ps1 -SuppressedOnly --> ParameterSetName=="File1".
4、Invoke-ScriptAnalyzer -Path C:\build.ps1 -IncludeSuppressions --> ParameterSetName=="File2".
5、Invoke-ScriptAnalyzer -ScriptDefinition $tmp --> ParameterSetName=="ScriptDefinition1".
6、Invoke-ScriptAnalyzer -ScriptDefinition $tmp -SuppressedOnly --> ParameterSetName=="ScriptDefinition1"
7、Invoke-ScriptAnalyzer -ScriptDefinition $tmp -IncludeSuppressions -->ParameterSetName=="ScriptDefinition2"
LMK if you have any suggestions, Thanks!
| /// <summary> | ||
| /// ShowAll: Show the suppressed and non-suppressed message | ||
| /// </summary> | ||
| [Parameter(Mandatory = false)] |
There was a problem hiding this comment.
correct, if it's not mandatory, that should just be omitted. if SuppressedOnly should not be used with this, then parameter sets should be used (and it does get complicated). Although it doesn't look to complicated at the moment it would mean an additional 2 parameter sets.
|
@bergmeister could you also please take a look at this? |
|
@t-lipingma I've implemented the suppression combination part of this work in #1699. |
bergmeister
left a comment
There was a problem hiding this comment.
Interesting idea. I wonder if it would also be useful to the user if the DiagnosticRecord gets an additional property whether it would've been suppressed so that the user does not need to run pssa twice and do logic on two sets of records.
PR Summary
Fixes #1691.
Output
Suppressionsto sarif result file.Add a list of suppressions for each diagnostic.
Add -IncludeSuppressions parameter for output all violations including suppressed one.
Add
Kindattribute in RuleSuppression.Add test cases for those changes.
PR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.