-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Add ValidatePropertyAttribute for duck-typing parameters #9622
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
Add ValidatePropertyAttribute for duck-typing parameters #9622
Conversation
|
@vexx32, your last commit had 3 failures in Unable to find type [System.Management.Automation.ValidateProperty].
At D:\a\1\s\test\powershell\Language\Parser\TypeAccelerator.Tests.ps1:303 char:35Tests for parameter binding.Default value conversion tests.ValidateScript can use custom ErrorMessage Expected strings to be the same, but they were different.
Expected length: 80
Actual length: 82
Strings differ at index 60.
Expected: '...led '$_ -g...'
But was: '...led ' $_ -...'
at <ScriptBlock>, D:\a\1\s\test\powershell\Language\Scripting\ParameterBinding.Tests.ps1: line 391
391: $err.Exception.Message | Should -BeExactly "Cannot validate argument on parameter 'p'. Item '2' failed '`$_ -gt 2' validation"Tests for parameter binding.Default value conversion tests.ValidateProperty can use custom ErrorMessage Expected strings to be the same, but they were different.
Expected length: 79
Actual length: 111
Strings differ at index 60.
Expected: '...led 'a,b' ...'
But was: '...led '[Syst...'
at <ScriptBlock>, D:\a\1\s\test\powershell\Language\Scripting\ParameterBinding.Tests.ps1: line 413
413: $err.Exception.Message | Should -BeExactly "Cannot validate argument on parameter 'p'. Item '2' failed 'a,b' property check" |
📝 Fix typo 🔧 add in type names to tests
|
@vexx32, your last commit had 2 failures in Expected 99, but got 100.
at <ScriptBlock>, /Users/vsts/agent/2.150.3/work/1/s/test/powershell/Language/Parser/TypeAccelerator.Tests.ps1: line 445
445: $TypeAccelerators.Count | Should -Be $totalAcceleratorsTests for parameter binding.Default value conversion tests.ValidateScript can use custom ErrorMessage Expected strings to be the same, but they were different.
Expected length: 80
Actual length: 82
Strings differ at index 60.
Expected: '...led '$_ -g...'
But was: '...led ' $_ -...'
at <ScriptBlock>, /Users/vsts/agent/2.150.3/work/1/s/test/powershell/Language/Scripting/ParameterBinding.Tests.ps1: line 391
391: $err.Exception.Message | Should -BeExactly "Cannot validate argument on parameter 'p'. Item '2' failed '`$_ -gt 2' validation" |
|
@vexx32, your last commit had 2 failures in Expected 104, but got 105.
at <ScriptBlock>, D:\a\1\s\test\powershell\Language\Parser\TypeAccelerator.Tests.ps1: line 445
445: $TypeAccelerators.Count | Should -Be $totalAcceleratorsTests for parameter binding.Default value conversion tests.ValidateScript can use custom ErrorMessage Expected strings to be the same, but they were different.
Expected length: 80
Actual length: 82
Strings differ at index 60.
Expected: '...led '$_ -g...'
But was: '...led ' $_ -...'
at <ScriptBlock>, D:\a\1\s\test\powershell\Language\Scripting\ParameterBinding.Tests.ps1: line 391
391: $err.Exception.Message | Should -BeExactly "Cannot validate argument on parameter 'p'. Item '2' failed '`$_ -gt 2' validation" |
|
@vexx32, your last commit had 2 failures in Expected 99, but got 100.
at <ScriptBlock>, /home/vsts/work/1/s/test/powershell/Language/Parser/TypeAccelerator.Tests.ps1: line 445
445: $TypeAccelerators.Count | Should -Be $totalAcceleratorsTests for parameter binding.Default value conversion tests.ValidateScript can use custom ErrorMessage Expected strings to be the same, but they were different.
Expected length: 80
Actual length: 82
Strings differ at index 60.
Expected: '...led '$_ -g...'
But was: '...led ' $_ -...'
at <ScriptBlock>, /home/vsts/work/1/s/test/powershell/Language/Scripting/ParameterBinding.Tests.ps1: line 391
391: $err.Exception.Message | Should -BeExactly "Cannot validate argument on parameter 'p'. Item '2' failed '`$_ -gt 2' validation" |
|
@vexx32 Please move all formatting in new PR. |
|
Wasn't intentional, VS Code got snarky, it seems. 🙂 I'll do a quick pass in a separate PR for just formatting to help with that. |
|
Calling out specific feedback on the related issue, here. |
|
Cheers, @KirkMunro! @iSazonov did a full style pass for |
|
@vexx32 Please rebase and resolve failures. |
|
@vexx32 Coming back to this one (since it just jumped into view again in my email watch list for this repo), and forgive me if I already asked this, but part of me wonders "why?" for this PR. We can already specify a custom type name in an attribute (e.g. For example: |
|
Have you heard of duck typing? Objects with similar property names may often have similar design intents despite coming from different sources. This kind of system allows folks to design a command with a particular input in mind and then to just make it work. Though, coming back to this now, it occurs to me that you kind of already can do this, at least with I'll rebase this soon, I have a few things to get to. |
|
@PoshChan please remind me in 9 days. |
|
I know duck typing very well, yes, but I still think the community should learn to use
|
|
I mean, using ValueFromPipelineByPropertyName needs 1 parameter per property. That can get pretty darn messy when you want to validate say 10 or so properties, especially if you need to take other parameters that aren't meant to come from the object itself. Results in some unfortunately messy design patterns... :/ |
|
On the flipside to that, if you're really checking 10 or so properties I'd argue you should be using a custom PSTypeName and checking for that type name instead. 👀 |
|
That's true, but there's no real guarantee that the object has been constructed properly in that instance, so that too only feels like a half-solution to the problem. |
|
If you're accepting a custom object as input, you should be receiving that custom object as output from somewhere else. Is that a fair statement? That's the only real way to get anything close to a guarantee. Otherwise what about the types of the values on the members in those objects...those could cause all sorts of issues in a command. No matter what, when you're working with custom objects, there is potential for an issue somewhere, but isn't that a design issue for the command author to think about? I don't really think someone shouldn't be writing commands that take a custom object as pipeline input with the expectation that the scripter will create the custom object, in which case the command they get that object from can give it to them with PSTypeName already set. For individual property validation, we have Something that might help me find this less questionable is a specific, non-contrived example where the issue you're trying to solve here is actually a problem in PowerShell today that would best be solved by this new attribute. Do we have one of those in a PowerShell issue somewhere? |
|
@daxian-dbw Ping.. |
|
@vexx32 can you resolve the merge conflict |
|
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 30 days. It will be closed if no further activity occurs within 10 days of this comment. |
PR Summary
Adds
[ValidateProperty()]attribute for duck-typing parameters, allowing function and cmdlet authors to accept arbitrary PSObject input provided the object(s) provides at least the specified named properties.PR Context
Resolves #7835. See issue for context.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.