Enable suppression of custom rules when used together with -IncludeDefaultRules #1245
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bergmeister
commented
May 30, 2019
bergmeister
commented
May 30, 2019
| && (ScriptAnalyzer.Instance.ExternalRules != null | ||
| && ScriptAnalyzer.Instance.ExternalRules.Count(item => String.Equals(item.GetFullName(), _ruleName, StringComparison.OrdinalIgnoreCase)) == 0) | ||
| && (ScriptAnalyzer.Instance.DSCResourceRules != null | ||
| && ScriptAnalyzer.Instance.DSCResourceRules.Count(item => String.Equals(item.GetName(), _ruleName, StringComparison.OrdinalIgnoreCase)) == 0)) |
There was a problem hiding this comment.
As one can see in the combination of all those statements is that we do not know whether the rule name to be suppressed is of type custom rule (where the rule name cannot be determined at design time) or not. Therefore it is not possible to make a statement whether the given rule name will be in one of the returned DiagnosticRecords, hence why the whole check is being removed.
rjmholt
approved these changes
May 30, 2019
JamesWTruher
approved these changes
Jun 7, 2019
There was a problem hiding this comment.
this looks good to me, did @Jaykul have an opportunity to comment on the tests to determine whether they captured his issue?
|
I just sent @Jaykul and @ChrisLGardner a message on slack with a local build of |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Summary
Fixes #1237
cc @Jaykul
In PSSA 1.17.1 and 1.18.1 I went through 3 test cases of using the following expressions as a custom rule name returned in the
DiagnosticRecordof a custom rule:$MyInvocation.MyCommand.Name(the name of the function name that returns theDiagnosticRecord)'$MyInvocation.InvocationName(same as above but fully qualified, i.e. module name pre-pended to itAll those scenarios are possible in both versions but when the
-IncludeDefaultRulesswitch was used, then in PSSA 1.17.1 the 2nd test case and in PSSA 1.18.0 the 1st and 3rd test case were not working because PSSA throws a red-herring error. This PR makes PSSA not throw an error any more and therefore enabling all 3 scenarios.The check that used to throw the error tried to check the rule name in the suppression against the available rule names. Unfortunately the conditional logic itself is a bit broken (this is why the errors only surfaced when the
-IncludeDefaultRuleswas used due to properties likeScriptAnalyzer.Instance.ScriptRulesnot being null any more). The problem with the check is that only at runtime will we know the returnedRuleNameof theDiagnosticRecord. People are encouraged to use expressions that mirror the function name that returns theDiagnosticRecordso that there is a match with whatGet-ScriptAnalyzerreturns but the reality is that people can return what they want and actively want to do so, see here. Therefore the check is removed as we cannot determine at this point in time if there is a match or not.Comparing the functionality with C#: the compiler similarly cannot throw an error either when a suppression attribute does not match an existing code style rule violation, therefore we are removing a feature that was never meant to be complete.
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.