introduce Command.Parse(CommandLineConfiguration) #2062

adamsitnik · 2023-02-20T14:08:37Z

Initially, I just wanted to extend Symbol with Parse(string[], CommandLineConfiguration).

I've realized that the existing Argument.Parse and Option.Parse methods have side effects, namely creating a RootCommand for given option/argument which sets the Symbol parent:

command-line-api/src/System.CommandLine/SymbolExtensions.cs

Lines 86 to 88 in 704e640

    
           static RootCommand Create(Symbol notCommand) 
        
               => notCommand is Option option 
        
                   ? new RootCommand { option }

command-line-api/src/System.CommandLine/ChildList.cs

Lines 37 to 41 in 6462288

    
           public void Add(T item) 
        
           { 
        
               item.AddParent(_parent); 
        
               _children.Add(item); 
        
           }

Since these methods were used only for testing and can be very easily replaced with explicit one liner I've decided to remove them:

- argument.Parse("x")
+ new RootCommand { argument }.Parse("x")

After doing that I've just moved CommandExtensions.Parse to Command to make it easier to discover (and follow BCL guidelines).

When I wanted to make Parser a static class, I've realized that we had two different default configs:

command-line-api/src/System.CommandLine/SymbolExtensions.cs

Line 41 in 704e640

parser = new Parser(new CommandLineConfiguration(root));

command-line-api/src/System.CommandLine/SymbolExtensions.cs

Line 54 in 704e640

parser = new CommandLineBuilder(root).UseDefaults().Build();

I've decided to use the same default config everywhere (the one that uses UseDefaults) and CompletionTests tests started failing, as they were not expecting the suggestions to contain --help and --version (global options added by default in the default config). I've just fixed the tests by passing the "simple" config in explicit way.

Another change that I've made was making Command a mandatory argument for CommandLineBuilder ctor:

command-line-api/src/System.CommandLine/Builder/CommandLineBuilder.cs

Lines 26 to 28 in 704e640

    
           public CommandLineBuilder(Command? rootCommand = null) 
        
           { 
        
               Command = rootCommand ?? new RootCommand();

I did that because it was making it easy to fall into a trap of not passing the existing command to the ctor and ending up with "disconnected" root command being used by the config:

RootCommand command = new()
{
    new Option<bool>(new[] { "--bool", "-b" }, "Bool option");
};

var builder = new CommandLineBuilder(); // not passing the command we already have, BUG
return builder.UseDefaults().Build().Invoke(args);

Since Command.Parse accepts CommandLineConfiguration and the class is not mutable yet, I wanted to make it easier to discover the builder type, so I've added:

public class CommandLineConfiguration
{
     public static CommandLineBuilder CreateBuilder(Command rootCommand);
}

I've also moved a bunch of extension methods to the types they were extending (BCL guideline).

I hope that all these changes are going to improve the discoverability of:

configuration and its customization
the separation of parsing and execution

fixes #2054
makes a progress toward #2057

…d setting it as a Parent of given Argument

…setting it as a Parent of given Argument

…scover

…ser instance anymore

…ndLineBuilder itself, make it partial class to reduce diff size

…has all we need

adamsitnik · 2023-02-20T14:33:10Z

src/System.CommandLine.Benchmarks/CommandLine/Perf_Parser_NestedCommands.cs

-        public void SetupParser()
-        {
-            SetupRootCommand();
-            _testParser = new Parser(_rootCommand);


Parser is now static, we can't benchmark it's creation ;)

adamsitnik · 2023-02-20T14:36:43Z

src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs

-    /// Provides extension methods for <see cref="CommandLineBuilder"/>.
-    /// </summary>
-    public static class CommandLineBuilderExtensions
+    public partial class CommandLineBuilder


to keep the diff smaller, I've decided to make the class partial rather than moving all the code to CommandLineBuilder

adamsitnik · 2023-02-20T14:40:02Z

src/System.CommandLine/Builder/CommandLineBuilder.cs

@@ -12,10 +12,10 @@ namespace System.CommandLine
    /// <summary>
    /// Enables composition of command line configurations.
    /// </summary>
-    public class CommandLineBuilder 
+    public partial class CommandLineBuilder 


this type was building a parser, but it was called CommandLineBuilder. Now it could be called CommandLineConfigurationBuilder, but this name is long and I expect that in the near future we are going to start using Cli name prefixes for types, so I've decided to keep the name for now.

CommandLineConfiguration => CliConfiguration CommandLineBuilder => CliConfigurationBuilder

Our hope is to remove the CommandLineBuilder class in any case, yes?

Our hope is to remove the CommandLineBuilder class in any case, yes?

The tricky part is to expose only properties, so we can configure everything by just using the setters and not needing fluent APIs. I am currently working on implementing Directive symbol and it looks very promising in that regard.

src/System.CommandLine/Parsing/Parser.cs

src/System.CommandLine.Benchmarks/CommandLine/Perf_Parser_NestedCommands.cs

src/System.CommandLine.Tests/ArgumentTests.cs

src/System.CommandLine.Tests/CommandTests.cs

src/System.CommandLine.Tests/ParseResultTests.cs

src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs

src/System.CommandLine/Parsing/Parser.cs

Co-authored-by: Kevin B <Keboo@users.noreply.github.com>

* use name "arguments" rather than "args" * don't accept null * accept IReadOnlyList rather than array

src/System.CommandLine/Command.cs

# 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.Tests/ResourceLocalizationTests.cs # src/System.CommandLine/Builder/CommandLineBuilder.cs # src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs # src/System.CommandLine/Invocation/InvocationContext.cs

adamsitnik added 9 commits February 18, 2023 21:31

remove Argument.Parse as it has side effects: creating RootCommand an…

3206f7b

…d setting it as a Parent of given Argument

remove Option.Parse as it has side effects: creating RootCommand and …

816aa60

…setting it as a Parent of given Argument

Move CommandExtensions.Parse to Command.Parse to make it easier to di…

c904e8f

…scover

don't store a reference to Parser

a2ffb8e

rename parser local variable to config in all tests as it's not a par…

11d70c6

…ser instance anymore

move all extension methods from CommandLineBuilderExtensions to Comma…

c5ceb80

…ndLineBuilder itself, make it partial class to reduce diff size

remove ParseResultExtensions.HasOption, as ParseResult.FindResultFor …

24e8d38

…has all we need

move ParseResultExtensions.Invoke* to ParseResult

3d503bf

move CommandLineConfigurationExtensions to CommandLineConfiguration

36d81b1

adamsitnik commented Feb 20, 2023

View reviewed changes

src/System.CommandLine/Parsing/Parser.cs Outdated Show resolved Hide resolved

adamsitnik marked this pull request as ready for review February 20, 2023 14:44

adamsitnik requested review from Keboo and jonsequitur February 20, 2023 14:44

Keboo reviewed Feb 20, 2023

View reviewed changes

adamsitnik and others added 2 commits February 20, 2023 22:42

Apply suggestions from code review

1184bfc

Co-authored-by: Kevin B <Keboo@users.noreply.github.com>

Apply suggestions from code review

5f22518

Keboo approved these changes Feb 20, 2023

View reviewed changes

unify Command.Parse and Parser.Parse:

803b1d4

* use name "arguments" rather than "args" * don't accept null * accept IReadOnlyList rather than array

jonsequitur reviewed Feb 22, 2023

View reviewed changes

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

adamsitnik added 2 commits February 22, 2023 21:20

address code review feedback: rename arguments to args

534c274

jonsequitur approved these changes Feb 22, 2023

View reviewed changes

adamsitnik merged commit 1e15ab1 into dotnet:main Feb 22, 2023

adamsitnik deleted the commandParse branch February 22, 2023 22:07

KalleOlaviNiemitalo mentioned this pull request Feb 28, 2023

Move Invoke* and Parse extension methods to their extended Symbol types #1905

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

introduce Command.Parse(CommandLineConfiguration) #2062

introduce Command.Parse(CommandLineConfiguration) #2062

Uh oh!

adamsitnik commented Feb 20, 2023 •

edited

Loading

Uh oh!

adamsitnik Feb 20, 2023

Uh oh!

adamsitnik Feb 20, 2023

Uh oh!

adamsitnik Feb 20, 2023

Uh oh!

jonsequitur Feb 21, 2023 •

edited

Loading

Uh oh!

adamsitnik Feb 21, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	static RootCommand Create(Symbol notCommand)
	=> notCommand is Option option
	? new RootCommand { option }

	public void Add(T item)
	{
	item.AddParent(_parent);
	_children.Add(item);
	}

	public CommandLineBuilder(Command? rootCommand = null)
	{
	Command = rootCommand ?? new RootCommand();

introduce Command.Parse(CommandLineConfiguration) #2062

introduce Command.Parse(CommandLineConfiguration) #2062

Uh oh!

Conversation

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

Uh oh!

Uh oh!

adamsitnik Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

adamsitnik Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

adamsitnik Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

jonsequitur Feb 21, 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!

adamsitnik Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamsitnik commented Feb 20, 2023 •

edited

Loading

jonsequitur Feb 21, 2023 •

edited

Loading