Add null check for args in CommandLineParser #13451
Conversation
iSazonov
added
the
CL-General
|
While we are here maybe add a comment like "The 'args' can not be null and can not contain null elements." in XML summary for all public methods where it is used? |
| { | ||
| if (args[i] is null) | ||
| { | ||
| throw new ArgumentNullException("The 'args' array contains a null element."); |
There was a problem hiding this comment.
The message should be put in .resx file. Make the message to "The specified arguments should not contain null elements.". Also, please pass in the parameter name to ArgumentNullException.
There was a problem hiding this comment.
No exceptions in the file use Resx file.
Please confirm using Resx file in the case.
There was a problem hiding this comment.
I think you need to differentiate an exception thrown due to user action from an exception that's thrown due to internal error only.
The InvalidOperationException thrown when _dirty == true can happen only when there is an internal error -- the parser gets reused somehow. It shouldn't happen and is not user facing.
The ArgumentNullException thrown here only because a user action -- the user pass in invalid args. It's user facing. For user-facing exceptions, the message should be localized eventually.
There was a problem hiding this comment.
I see your point. The issue could be raised (1) only in host application, (2) only if the host application developer constructs args with null element. So it is not "end user action" but a bug in the host application that is "internal error" case too. It looks the same as if we passed a bug with _dirty and an user catches this edge case with non-localized message. Thoughts?
There was a problem hiding this comment.
Anyone using a PowerShell public API is an end user. An internal error is an error that cannot be triggered by an end user. In this case, an end user cannot cause the argument parser to be reused.
I hope this makes it more clear to you.
There was a problem hiding this comment.
Thanks for clarify!
Done.
ghost
added
Waiting on Author
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
@daxian-dbw please update your review |
|
Review updated: #13451 (comment) |
src/Microsoft.PowerShell.ConsoleHost/resources/CommandLineParameterParserStrings.resx
Outdated
Show resolved
Hide resolved
…meterParserStrings.resx Co-authored-by: Robert Holt <rjmholt@gmail.com>
|
🎉 Handy links: |
3 participants
PR Summary
Throw if
argsarray contains a null.PR Context
#13429 (comment)
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.