make Symbol.Name mandatory, non-nullable and readonly #2067

adamsitnik · 2023-02-24T21:19:55Z

The goal of this PR it to make Name a mandatory, readonly and non empty value for every Symbol.

We need that to implement #2052: ParseResult.GetValue<T>(string name), which is going to allow our users to use the symbol name to fetch parsed value without the need of storing reference to Symbol.

Before:

Argument<string> pathArgument = new();
Option<bool> flagOption = new(new[] { "--flag", "-f" });

RootCommand command = new()
{
    pathArgument,
    flagOption
};

ParseResult parseResult = command.Parse(args);

string path = parseResult.GetValue(pathArgument);
bool flag = parseResult.GetValue(flagOption);

After

RootCommand command = new()
{
    new Argument<string>("path"),
    new Option<bool>("flag", new[] { "--flag", "-f" })
};

ParseResult parseResult = command.Parse(args);

string path = parseResult.GetValue("path");
bool flag = parseResult.GetValue("flag");

Another motivation is to remove Option.ArgumentHelpName which is confusing, as the fact that Option wraps an Argument is an implementation detail.
Last but not least, Argument being a parameter without a name, but having both Name and HelpName property was also confusing.

This required changes in help. So far, the value of first column was Argument.HelpName if present, completions (if any) or Argument.Name otherwise.

command-line-api/src/System.CommandLine/Help/HelpBuilder.Default.cs

Lines 73 to 86 in 1e15ab1

    
           var helpName = arg?.HelpName ?? string.Empty; 
        
           if (!string.IsNullOrEmpty(helpName)) 
        
           { 
        
               firstColumn = helpName!; 
        
           } 
        
           else if (completions.Length > 0) 
        
           { 
        
               firstColumn = string.Join("|", completions); 
        
           } 
        
           else 
        
           { 
        
               firstColumn = argument.Name; 
        
           }

Not everybody like that: #1976 and some of our users were setting Argument.HelpName just to avoid having all completions displayed in help.

Now the first column contains the Name (because it is always present), while second chooses Description. If description is not provided, it's using the completions.

fixes #1895
fixes #2038
fixes #2051

unblocks #2052 (required data will be always present)
unblocks #2053 (avoids conflicts related to ctors changes)

…nt and Option: * both always have Name specified, so first column can't contain enum values for Arguments with no name anymore * move enum values to second column, but only for symbols with no description

Keboo · 2023-02-28T17:04:13Z

src/System.CommandLine.Tests/ParseResultTests.cs

@@ -42,7 +42,7 @@ public void FindResult_can_be_used_to_check_the_presence_of_an_option()
        [Fact]
        public void FindResultFor_can_be_used_to_check_the_presence_of_an_implicit_option()
        {
-            var option = new Option<int>(new[] { "-c", "--count" }, () => 5);
+            var option = new Option<int>("--count", new[] { "-c", "--count" }, () => 5);


Suggested change

var option = new Option<int>("--count", new[] { "-c", "--count" }, () => 5);

var option = new Option<int>("count", new[] { "-c", "--count" }, () => 5);

Keboo · 2023-02-28T17:04:35Z

src/System.CommandLine.Tests/OptionTests.cs

        {
-            var option = new Option<string>(new[] { "myname", "m" });
+            var option = new Option<string>("--myname", new[] { "myname", "m" });


Suggested change

var option = new Option<string>("--myname", new[] { "myname", "m" });

var option = new Option<string>("myname", new[] { "myname", "m" });

Keboo · 2023-02-28T17:05:39Z

src/System.CommandLine.Tests/OptionTests.cs

@@ -70,39 +60,39 @@ public void A_prefixed_alias_can_be_added_to_an_option()
        [Fact]
        public void Option_aliases_are_case_sensitive()
        {
-            var option = new Option<string>(new[] { "-o" });
+            var option = new Option<string>("-o", new[] { "-o" });


Suggested change

var option = new Option<string>("-o", new[] { "-o" });

var option = new Option<string>("o", new[] { "-o" });

Keboo · 2023-02-28T17:05:51Z

src/System.CommandLine.Tests/OptionTests.cs

@@ -141,7 +131,7 @@ public void An_option_cannot_have_an_alias_consisting_entirely_of_whitespace()
        [Fact]
        public void Raw_aliases_are_exposed_by_an_option()
        {
-            var option = new Option<string>(new[] { "-h", "--help", "/?" });
+            var option = new Option<string>("--help", new[] { "-h", "--help", "/?" });


Suggested change

var option = new Option<string>("--help", new[] { "-h", "--help", "/?" });

var option = new Option<string>("help", new[] { "-h", "--help", "/?" });

Keboo · 2023-02-28T17:13:45Z

src/System.CommandLine/Option.cs

-            }
-
-            AddAlias(name);
+            AddAlias(name ?? throw new ArgumentNullException(nameof(name)));


I realize this matches the prior behavior, but are we sure we want to keep this? This looks like it will potentially cause un-expected behavior where the name is only sometimes added to the list of aliases.

Keboo · 2023-02-28T17:20:00Z

src/System.CommandLine/Symbol.cs

        {
+            if (string.IsNullOrWhiteSpace(name))
+            {
+                throw new ArgumentException("A name cannot be null, empty, or consist entirely of whitespace.");


Suggested change

throw new ArgumentException("A name cannot be null, empty, or consist entirely of whitespace.");

throw new ArgumentException("A name cannot be null, empty, or consist entirely of whitespace.", nameof(name));

Keboo · 2023-02-28T17:28:34Z

src/System.CommandLine.Tests/RootCommandTests.cs

        {
            var rootCommand = new RootCommand();
-            rootCommand.Name = "custom";
+            rootCommand.AddAlias("custom");

            rootCommand.Aliases.Should().BeEquivalentTo("custom", RootCommand.ExecutableName);
            rootCommand.Aliases.Should().BeEquivalentTo("custom", RootCommand.ExecutableName);


I realize these lines were not changed, but do we need both of these assertions? They appear to be duplicates.

jonsequitur · 2023-02-28T23:33:59Z

I'm hesitant on this change.

It changes the meaning of Option<T>.Name significantly and in way that will require code changes for every current user of System.CommandLine that, unlike our other breaking changes, won't be detectable by compiler errors.
It implies (based on past behaviors of all versions of System.CommandLine) that the Name property is no longer an identifier in the grammar. Take this example:
```
RootCommand command = new()
{
    new Argument<string>("path"),
    new Option<bool>("flag", new[] { "--flag", "-f" })
};

ParseResult parseResult = command.Parse(args);

string path = parseResult.GetValue("path");
bool flag = parseResult.GetValue("flag");
```
I assume we would treat this as an invalid command line:
```
> myapp flag
```
I can see how this change potentially allows us to disambiguate lookups by alias (e.g. do I look up using parseResult.GetValue("-f") versus parseResult.GetValue("--flag")?) but it only does so by introducing a third option "flag". I suspect people are still more like to use "--flag" as the lookup, especially given how long the API was used that way.

adamsitnik · 2023-03-01T21:03:28Z

It changes the meaning of Option<T>.Name significantly and in way that will require code changes for every current user of System.CommandLine that,

So far to get Option<T>.Name, we were searching for longest alias and removing the prefix. I am just proposing to make it a mandatory value provided by the user.

unlike our other breaking changes, won't be detectable by compiler errors.

You mean lack of compiler errors because all the ctors accept just strings?

I've a proposal for how we could simplify that: #2053 (comment)

that the Name property is no longer an identifier in the grammar

I am not sure if I understand. What do you mean by grammar? The parsing rules? Argument is a parameter with no name, so it was not used for parsing. Options name was not the actual alias, but a shortened version of it. So it was also not used for parsing.

Could you please elaborate more on that?

I can see how this change potentially allows us to disambiguate lookups by alias

I can add lookups by alias, especially since some products like SDK provide localized names for their symbols.

jonsequitur · 2023-03-01T21:59:44Z

Options name was not the actual alias, but a shortened version of it. So it was also not used for parsing.

Option.Name is set by specifying e.g. new Option<string>("--blah") and we've long regarded it as a bug that Option.Name returns it without the prefix. It's never been necessary to pass in aliases. So in that sense, the value specified in the constructor (which ideally should be used as the Name property without stripping the prefix) is an identifier in your program's command line grammar.

jonsequitur · 2023-03-01T22:03:00Z

unlike our other breaking changes, won't be detectable by compiler errors.

You mean lack of compiler errors because all the ctors accept just strings?

Yes.

I looked at the comment you mentioned and it includes a sample like this:

new Option<bool>("ucr", "--ucr", "--use-current-runtime")

I think the repetition of ucr here shouldn't be necessary. It solves an indexing problem by putting some extra trivia into the API that I think is a little more ceremony than we'd prefer.

Another approach might be to require a name parameter that's used, unmodified, to set the Name property, and require that that's used for indexing via GetValue(string name). Aliases are separate, optional, and not used for lookups.

Arguments don't need the alias concept and the name can be used for indexing pretty intuitively as is.

jonsequitur · 2023-03-01T22:07:09Z

I can add lookups by alias, especially since some products like SDK provide localized names for their symbols.

This sounds like a new scenario that deserves some discussion. Localizing actual identifiers sounds like a pretty uncommon case, and needing to know the localized identifier to perform a lookup sounds like it would invite a lot of bugs for API users.

adamsitnik · 2023-03-03T16:35:22Z

Closing in favor of #2073

adamsitnik added 3 commits February 24, 2023 21:54

make Symbol.Name a mandatory, non-nullable and readonly property

173e33e

fix tests

cdaf99c

remove HelpName and ArgumentHelpName, unify how help works for Argume…

0ceb0c1

…nt and Option: * both always have Name specified, so first column can't contain enum values for Arguments with no name anymore * move enum values to second column, but only for symbols with no description

KalleOlaviNiemitalo mentioned this pull request Feb 28, 2023

Does an argument with no name make sense? #1895

Closed

adamsitnik added 2 commits February 28, 2023 08:40

fix the test that passes locally but not on the CI

460315f

polishing after re-reading my own code on GH

6938496

adamsitnik marked this pull request as ready for review February 28, 2023 09:26

adamsitnik requested review from jonsequitur and Keboo February 28, 2023 09:26

adamsitnik mentioned this pull request Feb 28, 2023

introduce ParseResult.GetValue<T>(string name) #2070

Closed

Keboo reviewed Feb 28, 2023

View reviewed changes

jonsequitur mentioned this pull request Mar 1, 2023

Simplify custom parsers and default value factories #2053

Closed

adamsitnik mentioned this pull request Mar 3, 2023

Name and aliases separation #2073

Merged

adamsitnik closed this Mar 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

make Symbol.Name mandatory, non-nullable and readonly #2067

make Symbol.Name mandatory, non-nullable and readonly #2067

Uh oh!

adamsitnik commented Feb 24, 2023 •

edited

Loading

Uh oh!

Keboo Feb 28, 2023

Uh oh!

Keboo Feb 28, 2023

Uh oh!

Keboo Feb 28, 2023

Uh oh!

Keboo Feb 28, 2023

Uh oh!

Keboo Feb 28, 2023

Uh oh!

Keboo Feb 28, 2023

Uh oh!

Keboo Feb 28, 2023

Uh oh!

jonsequitur commented Feb 28, 2023

Uh oh!

adamsitnik commented Mar 1, 2023

Uh oh!

jonsequitur commented Mar 1, 2023

Uh oh!

jonsequitur commented Mar 1, 2023 •

edited

Loading

Uh oh!

jonsequitur commented Mar 1, 2023 •

edited

Loading

Uh oh!

adamsitnik commented Mar 3, 2023

Uh oh!

Uh oh!

	var helpName = arg?.HelpName ?? string.Empty;

	if (!string.IsNullOrEmpty(helpName))
	{
	firstColumn = helpName!;
	}
	else if (completions.Length > 0)
	{
	firstColumn = string.Join("\|", completions);
	}
	else
	{
	firstColumn = argument.Name;
	}

	var option = new Option<int>("--count", new[] { "-c", "--count" }, () => 5);
	var option = new Option<int>("count", new[] { "-c", "--count" }, () => 5);

	var option = new Option<string>("--myname", new[] { "myname", "m" });
	var option = new Option<string>("myname", new[] { "myname", "m" });

	var option = new Option<string>("-o", new[] { "-o" });
	var option = new Option<string>("o", new[] { "-o" });

	var option = new Option<string>("--help", new[] { "-h", "--help", "/?" });
	var option = new Option<string>("help", new[] { "-h", "--help", "/?" });

	throw new ArgumentException("A name cannot be null, empty, or consist entirely of whitespace.");
	throw new ArgumentException("A name cannot be null, empty, or consist entirely of whitespace.", nameof(name));

make Symbol.Name mandatory, non-nullable and readonly #2067

make Symbol.Name mandatory, non-nullable and readonly #2067

Uh oh!

Conversation

adamsitnik commented Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Keboo Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

Keboo Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

Keboo Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

Keboo Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

Keboo Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

Keboo Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

Keboo Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

jonsequitur commented Feb 28, 2023

Uh oh!

adamsitnik commented Mar 1, 2023

Uh oh!

jonsequitur commented Mar 1, 2023

Uh oh!

jonsequitur commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonsequitur commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamsitnik commented Mar 3, 2023

Uh oh!

Uh oh!

adamsitnik commented Feb 24, 2023 •

edited

Loading

jonsequitur commented Mar 1, 2023 •

edited

Loading

jonsequitur commented Mar 1, 2023 •

edited

Loading