Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upEnable suppression of custom rules when used together with -IncludeDefaultRules #1245
Conversation
| && (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)) |
This comment has been minimized.
This comment has been minimized.
bergmeister
May 30, 2019
•
Author
Collaborator
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.
|
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.
bergmeister commentedMay 30, 2019
•
edited
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.