-
Notifications
You must be signed in to change notification settings - Fork 382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow suppression of PSUseSingularNouns for specific function #1903
Allow suppression of PSUseSingularNouns for specific function #1903
Conversation
What's the status of this PR?? I need to be able to suppress this rule. Generally, this is a good rule. But, it's flagging the word 'Data' as "plural" even though that word is both Singular and Plural. |
Waiting for review. Same as #1896. Not sure what's going with the repo really. Only docs updates last 12 months. Would make sense if there was progress on PSSAv2, but IIRC that's also on hold. 😞 |
@fflaten Yup, more of a question directed at the repo owners...your changes LGTM. |
Hopefully @bergmeister or @JamesWTruher can provide an update. |
@fflaten Quick question on this PR: shouldn't the use of the I ask this, because it's really strange, but, I was able to successfully suppress this rule on two of my functions without the implementation of this PR. But on one function, it will simply not suppress. (And on the two functions where I was able to suppress this rule, I did not need to use the |
OK, more information. I don't think this PR is needed to suppress this rule. Here's my exact situation: I have a PowerShell script that I was linting. There are 3 functions in it that were being flagged by this attribute. After applying the # SUCCESSFULLY Suppressed
function Get-QueueMessageMetadata {
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseSingularNouns', '')]
[CmdletBinding()]
Param ( <# Doesn't matter what the parameters are #> )
Begin { }
}
# FAILED to suppress, as written below
# The below two attributes did not work:
#[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseSingularNouns', '', Target='Get-QueueWithMessages')]
#[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseSingularNouns', '', Target='Get-QueueWithMessages', Scope='function')]
function Get-QueuesWithMessages {
[CmdletBinding()]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseSingularNouns', '')]
# The below _also_ did not work, which was moved from outside the function declaration to here
#[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseSingularNouns', '', Target='Get-QueueWithMessages')]
Begin { }
}
# SUCCESSFULLY Suppressed
function Start-EmptyQueuesAndStopServices {
[CmdletBinding()]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseSingularNouns', '')]
Param ( <# Doesn't matter what the actual params are #> )
Begin { }
} Can you spot why two of these were successfully suppressed while the middle one was not? Go ahead, give it another look, and then I'll divulge what I learned........ OK, I'll relieve your suspense if you haven't seen it yet. The middle function has no declared I'm not sure if this is expected behavior. I can't find in any of my resources that the Unless there's some other use case this PR was addressing, I don't think this PR is actually needed to allow for the suppression of this attribute; it already works. |
@fourpastmidnight There's a difference: inheritance. function Get-Elements
{
# This will suppress the rule everywhere inside the targeted function -> both Get-Elements and Get-Containers will be suppressed
[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseSingularNouns', '', Scope = 'Function', Target = 'Get-Elements')]
# This will only suppress Get-Elements but still report Get-Containers
[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseSingularNouns', 'Get-Elements')]
# All SuppressMessageAttributes must be above a param() block, both at file level or inside a scriptblock.
param()
function Get-Containers
{
param()
}
}
# Intentionally conflicting functions
# Will be suppressed
function Get-Elements
{
# This will only suppress Get-Elements
[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseSingularNouns', 'Get-Elements')]
param()
# Will NOT be suppressed
function Get-Containers
{
param()
}
}
# Will NOT be suppressed
function Get-Elements
{
# This will only suppress Get-Elements
[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseSingularNouns', 'Get-Containers')]
param()
# Will be suppressed
function Get-Containers
{
param()
}
} Multiple rules already supports You already figured most of this out, but some quick comments about the samples in your post:
It should work if you add it above |
Thanks for your comments. They are helpful to allow me to better understand how PowerShell, and specifically, PSSA is working with attributes. However, to the contrary; when I did not supply a suppression attribute for As you pointed out for the second one, I was missing the It is interesting to see that the attributes I supplied would suppress the enclosing function and all child functions. That I did not know/understand. Thanks again for helping me better understand PSSA (and PowerShell). I have many years of PowerShell under my belt, but I continue to learn new things about it every day! |
Regarding my question on the need for the |
It's inherited design/behavior from C#/.NET AFAIK. Attributes in general are placed above the element they should target/associate with, which is why you need
In the issue I mentioned in the last post a comment-based suppression method is suggested for PSSAv2 to get more granular control without this limitation, similar to pragma used in other languages.
It is. > function t { [CmdletBinding()] }
ParserError:
Line |
1 | function t { [CmdletBinding()] }
| ~~~~~~~~~~~~~~~~~
| Unexpected attribute 'CmdletBinding'.
> function t { [CmdletBinding()]param() }
# no error
Likewise. That's what makes it fun 😃 |
Regarding not needing Thanks again! I'll be more "dogmatic" when writing functions and always supply a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, although we will need additional documentation.
@sdwheeler - this will need some documentation eventually |
@JamesWTruher I am not sure what needs to be documented here. Can you elaborate? Can't all rules be suppressed (ie. was this a bug) or are there other rules that can't be suppressed? If there are rules that can't be suppressed, then we need a list. |
@sdwheeler @fflaten Documentation wise we already document here some examples of rules where one can suppress more specifically: https://learn.microsoft.com/en-us/powershell/utility-modules/psscriptanalyzer/using-scriptanalyzer?view=ps-modules#suppressing-rules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a suppression example to the rule info, which I think is all we need to merge this now
PR Summary
Enables suppression of PSUseSingularNouns for specific function.
PR Checklist
.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
to the beginning of the title and remove the prefix when the PR is ready.