make CommandLineConfiguration mutable, remove CommandLineBuilder #2107

adamsitnik · 2023-03-20T14:48:28Z

fixes #1907
fixes #1920
fixes #1922
fixes #1924
fixes #1925
fixes #1926
fixes #2034
closes #2046 (the last element from the TODO list)
fixes #2057
fixes #2111
fixes #2117

…<string> as it allows for less

it allows us for creating the host and everything else after parsing has finished UseCommandHandler must specify the handler for instance of a Command, not per type as there can be multiple commands of the same type

# Conflicts: # src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt # src/System.CommandLine.Tests/Invocation/InvocationPipelineTests.cs # src/System.CommandLine/Builder/CommandLineBuilder.cs # src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs # src/System.CommandLine/CommandLineConfiguration.cs

# Conflicts: # src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_Hosting_api_is_not_changed.approved.txt # src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt # src/System.CommandLine.Benchmarks/CommandLine/Perf_Parser_Directives_Suggest.cs # src/System.CommandLine.Benchmarks/CommandLine/Perf_Parser_TypoCorrection.cs # src/System.CommandLine.DragonFruit.Tests/ConfigureFromMethodTests.cs # src/System.CommandLine.DragonFruit/CommandLine.cs # src/System.CommandLine.Hosting.Tests/HostingHandlerTest.cs # src/System.CommandLine.Hosting/HostingAction.cs # src/System.CommandLine.Hosting/HostingExtensions.cs # src/System.CommandLine.Rendering.Tests/ViewRenderingTests.cs # src/System.CommandLine.Tests/Help/HelpBuilderTests.Customization.cs # src/System.CommandLine.Tests/Invocation/InvocationPipelineTests.cs # src/System.CommandLine.Tests/Invocation/TypoCorrectionTests.cs # src/System.CommandLine.Tests/ParseDirectiveTests.cs # src/System.CommandLine.Tests/SuggestDirectiveTests.cs # src/System.CommandLine.Tests/UseExceptionHandlerTests.cs # src/System.CommandLine.Tests/UseHelpTests.cs # src/System.CommandLine.Tests/VersionOptionTests.cs # src/System.CommandLine/Builder/CommandLineBuilder.cs # src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs # src/System.CommandLine/CommandLineConfiguration.cs # src/System.CommandLine/Help/HelpOption.cs # src/System.CommandLine/Help/VersionOption.cs # src/System.CommandLine/Invocation/InvocationPipeline.cs # src/System.CommandLine/Invocation/TypoCorrection.cs

…sing the config instance for config.RootCommand.Parse(args, $config)

adamsitnik · 2023-03-27T20:03:10Z

src/System.CommandLine/ChildList.cs

@@ -37,7 +38,35 @@ public ChildList(Command parent)
        public void Add(T item)
        {
            item.AddParent(_parent);
+
+            if (_children.Count > 0 && _parent is RootCommand)


RootCommand ctor adds HelpOption and VersionOption by default now. Users might want to customize it like that:

RootCommand rootCommand = new() { new VersionOption("-v", "-version"), };

In order to avoid duplicates, this method checks if a help option or version option already exists. If it does, it overrides it.

I think it might be less surprising to just ask people to remove the other instance, especially given that the HelpAction is also customizable and is probably the more common path to customizing help.

@jonsequitur you are right, I've removed this logic and updated the tests

adamsitnik · 2023-03-27T20:05:01Z

src/System.CommandLine/Parsing/StringExtensions.cs

-                    tokens.Add(
-                        directive.Name,
-                        new Token(directive.Name, TokenType.Directive, directive, Token.ImplicitPosition));
+                    tokens[directive.Name] = new Token(directive.Name, TokenType.Directive, directive, Token.ImplicitPosition);


some directives are enabled by default. To avoid issues when users are not aware of that and they also add them to the config, I've replaced Add (it throws on duplicates) with just an indexer (overrides if given key was already present)

adamsitnik · 2023-03-27T20:06:51Z

src/System.CommandLine/CommandLineConfiguration.cs

+            RootCommand = rootCommand ?? throw new ArgumentNullException(nameof(rootCommand));
+            Directives = new()
+            {
+                new SuggestDirective()


I was not sure which directives should be enabled by default, it's an open question.

I have not enabled all of them, as it would cause a perf hit for the users.

Though I understand not wanting to cause a perf hit. I have a slight preference towards have the user opt-out rather than needing to opt-in to enable them.

jonsequitur · 2023-03-28T00:05:31Z

src/System.CommandLine.Tests/VersionOptionTests.cs

+            RootCommand commandWithDuplicates = new()
+            {
+                new VersionOption(),
+                new VersionOption(),


Shouldn't this be an error because of the symbol identifier collision?

I think the expectation with the builder is that it's idempotent, but I don't think I have the same expectation with Command.Add since we're enforcing name and alias uniqueness.

…n add, with no custom logic for overrides

jonsequitur · 2023-03-28T15:41:07Z

src/System.CommandLine.Tests/VersionOptionTests.cs

-                new VersionOption("-v", "-version"),
-            };
+                if (rootCommand.Options[i] is VersionOption)
+                    rootCommand.Options[i] = new VersionOption("-v", "-version");


This is a general comment and doesn't need to block merging.

The API through beta4 treated the arguments to Option<T> such that new VersionOption("-v", "-version") would indicate -version is the description.

The beta5 API understands this as setting Name to -v and using -version as an alias. In general usage though, -version would be the name (being longer) and -v would be the alias.

Two observations:

I've found this to be a gotcha in upgrading my code.

This usage in tests, while perfectly valid, might mislead about the way in which the API is typically used.

adamsitnik added 9 commits March 16, 2023 17:33

ParseResult.UnmatchedTokens should return an array, not IReadOnlyList…

4fdf9d5

…<string> as it allows for less

introduce new HostingAction to avoid using middleware in Hosting

4e89f0c

it allows us for creating the host and everything else after parsing has finished UseCommandHandler must specify the handler for instance of a Command, not per type as there can be multiple commands of the same type

don't use GetAwaiter().GetResult() to block, make the tests async..

8d67421

remove Middleware

054106c

expose HelpOption and HelpAction, move help configuration to the action

d798e5f

Merge branch 'hosting' into removeIConsole

02fe77f

replace IConsole with two TextWriters in Configuration

7b206d2

make CommandLineConfiguration mutable, remove CommandLineBuilder

2dd5b76

This was referenced Mar 20, 2023

Remove InvocationContext #2108

Merged

expose HelpOption and HelpAction, move help configuration to the action #2102

Merged

CancelOnProcessTermination no longer enabled by default #2111

Closed

adamsitnik added 5 commits March 27, 2023 15:16

rename Out to Output

bff1f84

RootCommand should define Help and Version options by default

c907171

introduce CommandLineConfiguration.Parse to avoid issues when not pas…

9d74085

…sing the config instance for config.RootCommand.Parse(args, $config)

some polishing after reading the code again on GH

430f390

adamsitnik commented Mar 27, 2023

View reviewed changes

adamsitnik marked this pull request as ready for review March 27, 2023 20:15

adamsitnik requested review from jonsequitur and Keboo March 27, 2023 20:15

jonsequitur reviewed Mar 28, 2023

View reviewed changes

address code review feedback: Command.Options.Add should be just plai…

36c5f15

…n add, with no custom logic for overrides

jonsequitur reviewed Mar 28, 2023

View reviewed changes

jonsequitur approved these changes Mar 28, 2023

View reviewed changes

adamsitnik merged commit 4185c60 into dotnet:main Mar 28, 2023

adamsitnik deleted the mutableConfig branch March 28, 2023 15:51

elgonzo mentioned this pull request May 9, 2023

Invoking command via Parser doesn't write to console, whereas command.Invoke does #2188

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

make CommandLineConfiguration mutable, remove CommandLineBuilder #2107

make CommandLineConfiguration mutable, remove CommandLineBuilder #2107

Uh oh!

adamsitnik commented Mar 20, 2023 •

edited

Loading

Uh oh!

adamsitnik Mar 27, 2023

Uh oh!

jonsequitur Mar 28, 2023

Uh oh!

adamsitnik Mar 28, 2023

Uh oh!

adamsitnik Mar 27, 2023

Uh oh!

adamsitnik Mar 27, 2023

Uh oh!

Keboo Mar 27, 2023

Uh oh!

jonsequitur Mar 28, 2023

Uh oh!

jonsequitur Mar 28, 2023

Uh oh!

Uh oh!

make CommandLineConfiguration mutable, remove CommandLineBuilder #2107

make CommandLineConfiguration mutable, remove CommandLineBuilder #2107

Uh oh!

Conversation

adamsitnik commented Mar 20, 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!

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!

adamsitnik commented Mar 20, 2023 •

edited

Loading