Name and aliases separation #2073

adamsitnik · 2023-03-03T15:58:16Z

It's a second attempt (the first one was #2067). For motivation please go to #2053 (comment)

Changes:

Separation of Name and Aliases concepts. Name needs to be always provided in an explicit way and can not be empty or changed later. We don't remove prefixes from Name anymore (as is). Name is also not added to Aliases anymore.
Simplification of the Symbol constructors. Each symbol type provides now only a single public ctor. The ctor requires to provide only the mandatory data (name), everything else needs to be provided by using the properties.
Removal of SetDefaultValue, SetDefaultValueFactory methods, they have been repalced with a single property.
Removal of IdentifierSymbol type. The aliases are now represented as a single property. It makes it very easy to implement HiddenAliases feature. Once this PR gets merged I am going to create an issue for that (or find an existing one and add description)
Rename of Option.ArgumentHelpName to just HelpName (the fact that we are using an Argument is an implementation detail)

Breaking change: users who were using Option(string name, string description) ctor, are now going to hit a silent error, as compiler won't warn them that description has became an alias now.
We are soon going to rename Option to CliOpton and expect the end users to not hit the silent error (we are currently not releasing any packages to nuget.org).
Those who are going to hit this issue are most likely going to get an exception at runtime, as description typically contains whitespaces and aliases can't.

fixes #1895
fixes #2038
fixes #2051
fixes #2053

unblocks #2052 (required data will be always present)

adamsitnik · 2023-03-03T16:23:03Z

src/System.CommandLine/Argument.cs

-        /// Initializes a new instance of the Argument class.
-        /// </summary>
-        protected Argument()
+        private protected Argument(string name) : base(name, canContainWhitespaces: true)


since arguments name is not used for parsing, I've decided to keep allowing the whitespaces. I don't have a strong opinion about that.

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

src/System.CommandLine/Help/HelpBuilder.Default.cs

            }
+            else if (!argument.IsBoolean() && argument.CompletionSources.Count > 0)


this change fixes #2038

First of all we check HelpName, if it's not provided and there are some CompletionSources defined for given Argument, then we are getting the completions.

It now adds <> around HelpName unlike before.

Huh, no, it did add <> already in 2.0 beta 4.

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

Line 91 in 209b724

return $"<{firstColumn}>";

jonsequitur · 2023-03-06T16:52:48Z

src/System.CommandLine.Tests/CompletionTests.cs

-                new Option<string>("--three", "option three")
+                new Option<string>("--one") { Description = "option one" },
+                new Option<string>("--two") { Description = "option two" },
+                new Option<string>("--three") { Description = "option three" },


The change to the params constructor for aliases results in some extra verbosity here and I wonder if it's the right tradeoff, given that people using Option set a description much more often than aliases.

We have at least three ways of setting the aliases:

what we had so far, which required the user to allocate an array in an explicit way

new Option<bool>("--help", new [] { "-h", "-?" }, "description");

using params, which looks way nicer, but requires the description to be set via a property. TBH I am not sure if it's a bad thing, but it's very subjective.

new Option<bool>("--help", "-h", "-?") { Description = "description" };

using collection initalizer syntax. IMO it looks great but I would expect very few C# devs to know about this duck typing.

new Option<bool>("--help", "description") { Aliases = { "-h", "-?" } };

@jonsequitur if you believe it's worth it, I can remove the params ctor and revert this part of the changes. My personal preference would be to merge it in the new form and eventually revisit it after API design review.

My hesitation is that the more common case is now more verbose, and it will be a very large source-breaking change. The reasoning makes sense though. I'd be curious to hear more opinions.

I'd be curious to hear more opinions.

@KathleenDollard @Keboo @KalleOlaviNiemitalo what is your opinion on that?

I am going to merge the PR now, but please don't hesitate to share your feedback.

Another option you have is to treat the aliases as peers to one another when supplying them, but have the convention that the first in the array is the primary and the rest are aliases.

new Option<bool>(new[] { "--help", "-h", "-?" }, "description");

Or move the aliases to the end of the parameter list:

new Option<bool>("--help", "description", new[] { "-h", "-?" });

I'm not a fan of the params for the aliases regardless though. With new[] { } being as succinct as it is (compared to when C# was young), I think it's harder to justify params for something like this. And I concur with @jonsequitur's position that the more common case is now more verbose.

In my first example above, I meant to explicitly state that there'd still be the single-string overload too. Calling that out in case it wasn't inferred.

Option(string name, string description); Option(string[] nameAndAliases, string description);

src/System.CommandLine/Symbol.cs

src/System.CommandLine.Tests/OptionTests.cs

jonsequitur · 2023-03-06T17:27:56Z

src/System.CommandLine.Tests/OptionTests.cs

-                isDefault: true);
+            var option = new Option<int>("-x")
+            {
+                DefaultValueFactory = _ => 123


I'd love to come up with a better name for this.

src/System.CommandLine/Help/HelpBuilder.Default.cs

Co-authored-by: Jon Sequeira <[email protected]>

…mmand-line-api#2073

* Update dependencies from https://github.com/dotnet/command-line-api build 20230307.2 System.CommandLine From Version 2.0.0-beta4.23152.1 -> To Version 2.0.0-beta4.23157.2 * Adjust the code following breaking change in Option ctor by dotnet/command-line-api#2073 --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Gang Wang <[email protected]>

…6224) * Update dependencies from https://github.com/dotnet/command-line-api build 20230307.2 System.CommandLine From Version 2.0.0-beta4.23152.1 -> To Version 2.0.0-beta4.23157.2 * [main] Update dependencies from dotnet/command-line-api (#6222) * Update dependencies from https://github.com/dotnet/command-line-api build 20230307.2 System.CommandLine From Version 2.0.0-beta4.23152.1 -> To Version 2.0.0-beta4.23157.2 * Adjust the code following breaking change in Option ctor by dotnet/command-line-api#2073 --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Gang Wang <[email protected]> * adjust to changes in command line --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Gang Wang <[email protected]> Co-authored-by: YuliiaKovalova <[email protected]>

…ne-api (#6225) * Update dependencies from https://github.com/dotnet/command-line-api build 20230307.2 System.CommandLine From Version 2.0.0-beta4.23152.1 -> To Version 2.0.0-beta4.23157.2 * [main] Update dependencies from dotnet/command-line-api (#6222) * Update dependencies from https://github.com/dotnet/command-line-api build 20230307.2 System.CommandLine From Version 2.0.0-beta4.23152.1 -> To Version 2.0.0-beta4.23157.2 * Adjust the code following breaking change in Option ctor by dotnet/command-line-api#2073 --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Gang Wang <[email protected]> --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Gang Wang <[email protected]>

adamsitnik added 5 commits March 2, 2023 22:32

apply the changes to symbol types

22f4e8b

update tests

46b3509

making the tests pass

4ca6397

remove IdentifierSymbol type

a82cd00

solve the TODOs

35537e3

adamsitnik commented Mar 3, 2023

View reviewed changes

adamsitnik requested review from jonsequitur and Keboo March 3, 2023 16:33

adamsitnik mentioned this pull request Mar 3, 2023

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

Closed

jonsequitur reviewed Mar 6, 2023

View reviewed changes

src/System.CommandLine/Symbol.cs Outdated Show resolved Hide resolved

jonsequitur reviewed Mar 6, 2023

View reviewed changes

src/System.CommandLine.Tests/OptionTests.cs Outdated Show resolved Hide resolved

jonsequitur reviewed Mar 6, 2023

View reviewed changes

src/System.CommandLine/Help/HelpBuilder.Default.cs Outdated Show resolved Hide resolved

adamsitnik and others added 2 commits March 6, 2023 20:47

Apply suggestions from code review

6aa42f3

Co-authored-by: Jon Sequeira <[email protected]>

address code review feedback

d145d8d

jonsequitur approved these changes Mar 7, 2023

View reviewed changes

adamsitnik merged commit 7d3cd43 into dotnet:main Mar 7, 2023

adamsitnik deleted the nameAndAliasesSeparation branch March 7, 2023 18:25

GangWang01 added a commit to dotnet/templating that referenced this pull request Mar 9, 2023

Adjust the code following breaking change in Option ctor by dotnet/co…

41ded9d

…mmand-line-api#2073

adamsitnik mentioned this pull request Mar 28, 2023

IdentifierSymbol.AddAlias summary not useful in Intellisense #1845

Closed

This was referenced Mar 29, 2023

Consider a different return type for Symbol.Aliases #1945

Closed

--help no longer displays the option argument if Option.HelpName is not set #2128

Open

Honor user defined order of commands, subcommands and options #1215

Closed

KalleOlaviNiemitalo mentioned this pull request Jul 31, 2023

CommandHandler.Create won't map every Property #2253

Closed

KalleOlaviNiemitalo mentioned this pull request Aug 26, 2023

No way to distinguish between ParseArgument invoked for user-supplied input and default value #1287

Open

KalleOlaviNiemitalo mentioned this pull request Sep 2, 2023

dotnet build --help results in project evaluation and msbuild SDK resolution dotnet/sdk#27374

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Name and aliases separation #2073

Name and aliases separation #2073

Uh oh!

adamsitnik commented Mar 3, 2023 •

edited

Loading

Uh oh!

adamsitnik Mar 3, 2023

Uh oh!

jonsequitur Mar 6, 2023

Uh oh!

adamsitnik Mar 3, 2023

Uh oh!

KalleOlaviNiemitalo Mar 8, 2023

Uh oh!

KalleOlaviNiemitalo Mar 8, 2023 •

edited

Loading

Uh oh!

jonsequitur Mar 6, 2023

Uh oh!

adamsitnik Mar 6, 2023

Uh oh!

jonsequitur Mar 6, 2023

Uh oh!

adamsitnik Mar 6, 2023

Uh oh!

adamsitnik Mar 7, 2023

Uh oh!

jeffhandley Mar 8, 2023

Uh oh!

jeffhandley Mar 10, 2023

Uh oh!

Uh oh!

Uh oh!

jonsequitur Mar 6, 2023

Uh oh!

Uh oh!

Uh oh!

		}
		else if (!argument.IsBoolean() && argument.CompletionSources.Count > 0)

Name and aliases separation #2073

Name and aliases separation #2073

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adamsitnik commented Mar 3, 2023 •

edited

Loading

KalleOlaviNiemitalo Mar 8, 2023 •

edited

Loading