- Notifications
You must be signed in to change notification settings - Fork412
Refs #1074 - Error message "Required argument {ARGUMENT NAME} missing for command {commandName}..."#1993
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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
… {commandname}", should provide "Required argument {argumentname} missing for command: {commandname}" for better clarity as to which required argument is missing.| publicNonWindowsOnlyFactAttribute() | ||
| { | ||
| if(RuntimeEnvironment.OperatingSystemPlatform==Platform.Windows) | ||
| if(RuntimeEnvironment.OperatingSystemPlatform==Microsoft.DotNet.PlatformAbstractions.Platform.Windows) |
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.
That looks unrelated. There is a similar change elsewhere in this PR.
| /// </summary> | ||
| publicvirtualstringRequiredArgumentMissing(Argumentargument,SymbolResultsymbolResult)=> | ||
| symbolResultisCommandResult | ||
| ?GetResourceString(Properties.Resources.CommandRequiredArgumentMissing,argument.Name,symbolResult.Token().Value) |
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.
In the development version I think this should prefer HelpName if set.
| publicvirtualstringRequiredArgumentMissing(SymbolResultsymbolResult)=> | ||
| symbolResultisCommandResult | ||
| ?GetResourceString(Properties.Resources.CommandRequiredArgumentMissing,symbolResult.Token().Value) | ||
| ?GetResourceString(Properties.Resources.CommandRequiredArgumentMissing,"(unknown)",symbolResult.Token().Value) | ||
| :GetResourceString(Properties.Resources.OptionRequiredArgumentMissing,symbolResult.Token().Value); |
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.
Because#2041 made LocalizationResources internal, you can just delete this overload. Then there won't be an "(unknown)" string that might have to be localised.
TMUNYU commentedOct 27, 2023
Any eta for this PR? |
| <ItemGroup> | ||
| <InternalsVisibleToInclude="System.CommandLine.NamingConventionBinder" /> | ||
| <InternalsVisibleToInclude="System.CommandLine.Tests" /> |
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.
It's good to avoid making internals visible to tests.
There are unaddressed comments and merge conflicts that need to be resolved. |
Previously it was "Required argument missing for command: {commandname}" - now we have better clarity as to which argument is missing.