Skip to content
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

Refs #1074 - Error message "Required argument {ARGUMENT NAME} missing for command {commandName}..." #1993

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Drizin
Copy link

@Drizin Drizin commented Dec 13, 2022

Previously it was "Required argument missing for command: {commandname}" - now we have better clarity as to which argument is missing.

… {commandname}", should provide "Required argument {argumentname} missing for command: {commandname}" for better clarity as to which required argument is missing.
@@ -10,7 +10,7 @@ public class NonWindowsOnlyFactAttribute : FactAttribute
{
public NonWindowsOnlyFactAttribute()
{
if (RuntimeEnvironment.OperatingSystemPlatform == Platform.Windows)
if (RuntimeEnvironment.OperatingSystemPlatform == Microsoft.DotNet.PlatformAbstractions.Platform.Windows)

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>
public virtual string RequiredArgumentMissing(Argument argument, SymbolResult symbolResult) =>
symbolResult is CommandResult
? GetResourceString(Properties.Resources.CommandRequiredArgumentMissing, argument.Name, symbolResult.Token().Value)

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.

Comment on lines 104 to 107
public virtual string RequiredArgumentMissing(SymbolResult symbolResult) =>
symbolResult is CommandResult
? GetResourceString(Properties.Resources.CommandRequiredArgumentMissing, symbolResult.Token().Value)
? GetResourceString(Properties.Resources.CommandRequiredArgumentMissing, "(unknown)", symbolResult.Token().Value)
: GetResourceString(Properties.Resources.OptionRequiredArgumentMissing, symbolResult.Token().Value);

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
Copy link

TMUNYU commented Oct 27, 2023

Any eta for this PR?

@@ -52,6 +52,7 @@

<ItemGroup>
<InternalsVisibleTo Include="System.CommandLine.NamingConventionBinder" />
<InternalsVisibleTo Include="System.CommandLine.Tests" />
Copy link
Contributor

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.

@jonsequitur
Copy link
Contributor

There are unaddressed comments and merge conflicts that need to be resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants