-
Notifications
You must be signed in to change notification settings - Fork 7.2k
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
Fix ValidateRange Max Depth Allowed #16197
Conversation
Replaced int.MaxValue with maxDepthAllowed so that parameter validation works as expected.
Set ValidateRange MaxDepth to 100 explicitly.
Remove MaximumAllowedDepthReached and check for ParameterArgumentValidationError instead.
Remove BeginProcessing method as no longer needed for prerequisite checks.
src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/ConvertToJsonCommand.cs
Show resolved
Hide resolved
src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/ConvertToJsonCommand.cs
Show resolved
Hide resolved
@SteveL-MSFT can you please take a quick look? |
I wonder why do we change the check if we get right error message? Also see #3181 - I wonder about it too. |
The documentation is already up-to-date;
@iSazonov This is to fix the incorrect validate range attribute. While after fixing that, there is no need for explicit check. |
I had reverse logic to keep PowerShell custom error message. :-) No objections to have ValidateRange error message. |
Actually, there is an inconsistency in the doc. |
@daxian-dbw It supports a value of 0 but is it valid?
The supported range should probably start at 1, not 0. |
@sdwheeler It works on 7.2.0-preview.9, so I guess there was a change that made this work at some point. |
@daxian-dbw Ah, yeah. I think that was one of the things fixed. The docs for the cmdlet in 7.2 already list the range as |
Right, the 7.2 doc has the right content. Thanks @sdwheeler!
|
🎉 Handy links: |
PR Summary
Replace int.MaxValue with 100 so that parameter validation works as expected.
Remove BeginProcessing method. No longer required to do a prerequisite check on Max Depth. Parameter Argument Validation now used.
Update Json Tests.
PR Context
Fixes #15344
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).