Repository navigation
Add support for suppressing custom rules #1140
Description
Activity
Please give more detail what you are asking for. Are you saying it is not possible to suppress custom rules using attributes?
jegannathanmaniganadan commented
on Feb 14, 2019 ContributorAuthorYes, that is what the readme says https://github.com/PowerShell/PSScriptAnalyzer#suppressing-rules
Thanks. Hmm, I'll have to try and see, maybe this readme section is out of date. In general if you are sure about this we would definitely accept a PR for that.
jegannathanmaniganadan commented
on Feb 14, 2019 ContributorAuthorAFAICK, it never worked for custom rule. This can be a deal breaker for who enforces their organization coding standard using custom rules. There will definitely be legitimate use cases for suppressing custom rules IMO.
Hi, yes, I can confirm that this feature has not been implemented yet and I definitely see the value of it. I am not familiar with the code that does the suppression, therefore I don't know how involved it is to get it to work. Currently, no one is working on it, this repo doesn't really have a feature pipeline. Although I might approach it at some point (which could mean several months or even longer considering that the change would also need to be released) but if you really want this feature I suggest you try open a PR. We want to release a new version in about 1-2 months, therefore getting it in before could be useful to you. I would be happy to help out if you have questions or need help to finish it off once you have a first working prototype. Even just pointers to code that would need to be modified can help reduce the work for maintainers.
- changed the title
[-]Is there plan for extending the suppression feature for custom rules ?[/-][+]Add support for suppressing custom rules[/+]on Feb 16, 2019 jegannathanmaniganadan commented
on Feb 20, 2019 ContributorAuthorChristoph Bergmeister (@bergmeister)
If I get some help or pointers to where the code needs change, I can give it a try. I don't want to waste time understanding codes that are not relevant to this change.jegannathanmaniganadan commented
on Feb 21, 2019 ContributorAuthorChristoph Bergmeister (@bergmeister) This is not a feature, actually this is broken. custom rule suppression is actually working.
The rule name must be consistent with the rule function. Ideally, PSSA should handle this inconsistent rule name, but that is broken for some reason.
@manigandan-jegannathan-developer It seems the rule name of a custom rule is
FileNameofCustomRuleName\RuleName. When trying to suppress it, it cannot find the rule because it is missing the first part, therefore I started to look at this place where this happens and then worked my way up to the root cause. It wasn't too difficult at the end so I could even open a PR for that already (see below)jegannathanmaniganadan commented
on Mar 23, 2019 ContributorAuthorSo how should we suppress any rule now. I have been trying from the new version 1.18. Looks like #1145 broke the existing suppression mechanism also.
With 1.17.1 I was able to at least suppress a rule using the function name like below.
[Diagnostics.CodeAnalysis.SuppressMessageAttribute("Measure-VariableCase", '')]
param ()Now it is completely broken.
jegannathanmaniganadan commented
on Mar 23, 2019 ContributorAuthorUgg! why did we do this ? Now we have to pass like below ? This is more uglier than the previous one.
**1.18** [Diagnostics.CodeAnalysis.SuppressMessageAttribute("SampleRule\Measure-PSVariableCase", '')] param() **1.17.1** [Diagnostics.CodeAnalysis.SuppressMessageAttribute("Measure-PSVariableCase", '')] param()Is there an alternate way to have it shorter ? You can always set rule info to anything you want in [DiagnosticRecord]. At least previously we can set the rule name as Measure-PSVariableCase (basically function name) in the [DiagnosticRecord]. But now we need to set SampleRule\Measure-PSVariableCase as a rule and use the full name SampleRule\Measure-PSVariableCase in the suppression attribute.
In fact, I was expecting in the new PR, we will be able to suppress based on the rule name whatever we set the in [DiagnosticRecord] object. It does not have to rely on function name or anything else. Looks like that's not the case.
I am thinking now whether should I go with 1.18 :/.
This does not make any sense to me. I think we both agreed that in 1.17.1 it was not possible at all to suppress a custom rule?
The fully qualified name of the record is needed to unique identify it as another custom rule could define the same rule name. Please give a full example if you think there is some unneeded coupling or what you think is brokenjegannathanmaniganadan commented
on Mar 28, 2019 ContributorAuthorI think there was a miscommunication. If you look at my previous updates, I said that it is not broken. PSSA expects the rule name to match with the function name. That's something I expected to change.
I'd not want to name my PSSA rule as Measure-PSAvoidFoo, I'd rather prefer it as PSAvoidFoo. But now the problem is, most of our code has suppressmessageattribute as Measure-PSAvoidFoo. with this new version, we have to change SampleRule\Measure-PSAvoidFoo. That is my concern.
quoting it again,
@bergmeister This is not a feature, actually this is broken. custom rule suppression is actually working. The rule name must be consistent with the rule function. Ideally, PSSA should handle this inconsistent rule name, but that is broken for some reason.jegannathanmaniganadan commented
on Apr 27, 2019 ContributorAuthorChristoph Bergmeister (@bergmeister) I have couple of doubts with this,
- What should be my custom rule name, does it have to be in the same format used in suppression, like SampleRule\Measure-PSVariableCase? Basically what name I should ship in DiagnosticRecord.
- Also does the rule name must be the name of the function ? Or can I have any name ?
I kind of feel my suppression is not working with new version. So checking with you, appreciate your help with this
@manigandan-jegannathan-developer The test in my PR uses
ModuleName\FunctionNamein the suppression and$PSCmdlet.MyInvocation.InvocationNamefor the rule name when returning the diagnostic record, which should definitely work. I'd expect to just useModuleName\RuleNameto work as well, i.e. you just specify the rule name in the DiagnosticRecord instead of coupling it to the function name.jegannathanmaniganadan commented
on Apr 28, 2019 ContributorAuthor@manigandan-jegannathan-developer The test in my PR uses ModuleName\FunctionName in the suppression and $PSCmdlet.MyInvocation.InvocationName for the rule name when returning the diagnostic record, which should definitely work.
I have the similar set up, but suppression is not working for me. Am I missing anything ? Christoph Bergmeister (@bergmeister)
jegannathanmaniganadan commented
on Apr 30, 2019 ContributorAuthorChristoph Bergmeister (@bergmeister)
i.e. you just specify the rule name in the DiagnosticRecord instead of coupling it to the function name.
I really doubt this.
Have a look here for an example of a suppression of a simple custom rule (suppression is commented out to show warning first, just un-comment the suppression and you will see that it works:
https://github.com/bergmeister/PSHSummit2019/tree/master/3_Custom%20Rulesjegannathanmaniganadan commented
on May 1, 2019 ContributorAuthorThanks Christoph Bergmeister (@bergmeister) for all your help. I found the problem, I am not using $myinvocation.InvocationName in my custom rule. That's the ultimate problem which I have been telling from the beginning (see my previous comments.)
- Suppression works only when I use $myinvocation.InvocationName as a rule name in the [DiagnosticRecord], So either I should set rule name as,
- $myinvocation.InvocationName
or - CustomRule\FunctionName (now). Previously just the function name
- $myinvocation.InvocationName
- The reason I do not want to use my function name as a rule name is,
- Having rule name to something like Measure-AvoidFoo looks ugly. Of course I can have the function name to what ever convenient, but that's not the point. I have a custom rule to catch such violation and I can't myself violate it for the sake of this program to work.
- I have more custom rules, so I'd prefer stashing all these rule information in a json file, like rulename, severity, exceptions, fix. I'd also prefer having a wrapper that reads and generates the list of custom rules that I have, similar to Get-ScriptAnalyzerRule. Now that we have to have the rule name as a function name also for the suppression to work, we need to prepend the modulename, which now looks more uglier, like "CustomRule\Measure-Foo"
- The another problem is that, which name I should be having in PSScriptAnalyzerSettings file. I do not want include all the rules, so I'd prefer using -IncludeRules in my setting file with selected number of builtin rule + all my custom rules. The discrepancy here is that, I must give the Function Name, so that PSSA will include the corresponding custom rules. CustomRules\Measure-Foo would not work.
- Suppression works only when I use $myinvocation.InvocationName as a rule name in the [DiagnosticRecord], So either I should set rule name as,
jegannathanmaniganadan commented
on May 5, 2019 ContributorAuthorChristoph Bergmeister (@bergmeister) Just checking with you about my previous comment
You're right, it seems to be the function name that is used and not the rule name for suppression, hence why it works when using
$myinvocation.InvocationName. I don't see it as a big issue though, feel free to open a pull request for that. In order to avoid breaking changes, I am thinking it would be better to allow a rule name based suppression in addition to the current function name based one. The same applies to theIncludeRulesparameter together with custom rule names. About some of your other previous comments: If you think there is a case where something worked in 1.17.1 but not in 1.18.0 then please give a complete example, PRs to fix that would be welcome as well.
For my purposed, the functionality in PSSA is good enough, therefore I will probably not spend my efforts/time on enhancing this area, you will need to create a PR.Some more discussion is going on in #1237 about the inconsistency between
Get-ScriptAnalyzer/Invoke-ScriptAnalyzerand the different switches and what to use as the rule name. Making it consistent and backwards compatible with 1.17.1 again should be top priority for now. Please share here your specific use cases that broke from 1.17.1 to 1.18.0jegannathanmaniganadan commented
on May 21, 2019 ContributorAuthorThanks Christoph Bergmeister (@bergmeister). I hope I will find sometime to create a PR for this. This will be useful IMO. Thanks for all your work. Appreciate it :)
Is this something in the feature pipeline ?