-
Notifications
You must be signed in to change notification settings - Fork 401
Add UseSingleValueFromPipelineParameter Rule #2119
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
base: main
Are you sure you want to change the base?
Add UseSingleValueFromPipelineParameter Rule #2119
Conversation
You're right that most of the time someone does this, it's probably a mistake. But it is valid and there's a small use case of switching parameter sets based on the type of value received over the pipeline. So we can accept this as an optional rule, but it cannot be on by default. |
| [UseProcessBlockForPipelineCommand](./UseProcessBlockForPipelineCommand.md) | Warning | Yes | | | ||
| [UsePSCredentialType](./UsePSCredentialType.md) | Warning | Yes | | | ||
| [UseShouldProcessForStateChangingFunctions](./UseShouldProcessForStateChangingFunctions.md) | Warning | Yes | | | ||
| [UseSingleValueFromPipelineParameter](./UseSingleValueFromPipelineParameter.md) | Warning | Yes | | |
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.
Can you make this rule not enabled by default?
Thanks so much for looking at this Andy 😀 I don't follow currently and I'm keen to. If you've the time to explain that use-case, I'd really appreciate that. If not, don't worry I'll get this changed to optional as you suggest. The rule checks that, at most, one parameter per set accepts pipeline input by value (my comment ommited "set"). So, if you're changing parameter sets based on the pipeline input - that's fine, you'd still only be accepting value binding on one parameter in that set? function Test-ValueFromPipeline {
[CmdletBinding()]
param (
[Parameter(ParameterSetName = 'One', ValueFromPipeline)]
[string]
$Parameter1,
[Parameter(ParameterSetName = 'Two', ValueFromPipeline)]
[int]
$Parameter2
)
process {
"$($PSCmdlet.ParameterSetName) was used."
"Parameter1: $Parameter1!"
"Parameter2: $Parameter2!"
}
}
'8' | Test-ValueFromPipeline
# One was used.
# Parameter1: 8!
# Parameter2: 0!
8 | Test-ValueFromPipeline
# Two was used.
# Parameter1: !
# Parameter2: 8! When multiple parameters accept pipeline input by value - binding without and with type coercion is attempted for each. If the binding succeeds for multiple parameters then you have the same data bound to multiple parameters? I guess one parameter could be a string and another could be an int, bound by coercion if the string is a valid integer? Is the use-case you're talking about when coercion for one such type would fail? e.g. An function Test-ValueFromPipeline {
[CmdletBinding()]
param (
[Parameter(ValueFromPipeline)]
[System.Diagnostics.Process]
$Parameter1,
[Parameter(ValueFromPipeline)]
[int]
$Parameter2
)
process {
"Parameter1: $($Parameter1.Name)!"
"Parameter2: $Parameter2!"
}
}
Get-Process -Name 'Code' | Select -First 1 | Test-ValueFromPipeline
# Parameter1: Code!
# Parameter2: 0!
1990 | Test-ValueFromPipeline
# Parameter1: !
# Parameter2: 1990! You'd then make some check about If that's the case, why would the use of parameter sets not be encouraged? The 32 set limit? Complexity? |
@SeeminglyScience has the explanation (I was relaying it from a triage call). |
PR Summary
Adds a new rule,
UseSingleValueFromPipelineParameter
, which flags when multiple parameters in the same parameter set haveValueFromPipeline
set to true.When running something like:
I don't get any kind of error or warning, which I was surprised by.
Consistent behaviour across both 5.1 and 7.4.11.
Multiple parameters accepting pipeline input by value doesn't make sense (to me at least - maybe I'm missing a use-case?).
The above outputs:
Both parameters get the pipeline input bound.
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.