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

Announcing System.CommandLine 2.0 Beta 2 and the road to GA #1537

Open
jonsequitur opened this issue Dec 17, 2021 · 46 comments
Open

Announcing System.CommandLine 2.0 Beta 2 and the road to GA #1537

jonsequitur opened this issue Dec 17, 2021 · 46 comments

Comments

@jonsequitur
Copy link
Contributor

jonsequitur commented Dec 17, 2021

System.CommandLine 2.0 Beta 2

It's been a while since the last beta release of System.CommandLine. We’re happy to be able to say that it's almost time for a non-beta release of System.CommandLine 2.0. The library is stable in the sense of being robust and relatively bug free, but it was too easy to make mistakes with some of the APIs. In this release, we're providing improved APIs, offering a lightweight injection approach, making it easier for you to customize help, improving tab completion, working on documentation, and making big performance improvements. We’ve delayed going to GA because we want feedback on the changes. We anticipate this release to be the last before one or two stabilizing RCs and, at last, a stable 2.0 release.

These updates have required a number of breaking changes over the last few months. Rather than trickling them out, we’ve batched most of them up into a single release. The changes are too significant to jump into a release candidate without giving you time to comment. You can see the list here. The most important changes are summarized below. Please give the latest packages a try and let us know your thoughts.

Command handler improvements

Introducing System.CommandLine.NamingConventionBinder

Since the early days of the System.CommandLine 2.0 effort, one API has stood out as particularly troublesome, which is the binding of option and argument values to command handler parameters by name. In this release, we introduce a new approach (described below) and move the naming convention to a separate NuGet package, System.CommandLine.NamingConventionBinder.

Here's an example of the naming convention-based approach to binding, which you'll be familiar with if you've been using System.CommandLine Beta 1:

var rootCommand = new RootCommand
{
    new Option<int>("-i"),
    new Argument<string>("-s"),
};

rootCommand.Handler = CommandHandler.Create<int, string>((i, s) => { });

The parameters in the handler delegate will only be populated if they are in fact named i and s. Otherwise, they'll be set to 0 and null with no indication that anything is wrong. We thought this would be intuitive because this convention is similar to name-based route value binding in ASP.NET MVC. We were wrong. This has been the source of the majority of the issues people have had using System.CommandLine.
Moving the name-based binding APIs into a separate package encourages the use of the newer command handler APIs and will eventually make System.CommandLine trimmable. If you want to continue using them, you can find them in System.CommandLine.NamingConventionBinder. This package is where you'll also now find the support for convention-based model binding of complex types. If you want to continue using these APIs, do the following and everything will continue to work:

  • In your project, add a reference to System.CommandLine.NamingConventionBinder.
  • In your code, change references to the System.CommandLine.Invocation namespace to use System.CommandLine.NamingConventionBinder, where the CommandHandler.Create methods are now found. (There’s no longer a CommandHandler type in System.CommandLine, so after you update you’ll get compilation errors until you reference System.CommandLine.NamingConventionBinder.)

The new command handler API

The recommended way to set a command handler using the core System.CommandLine library now looks like this:

// This is the same example as above.
var option = new Option<int>("-i");
var argument = new Argument<string>("-s");
var rootCommand = new RootCommand
{
    option,
    argument
};

// This is new!
rootCommand.SetHandler(
    (int someInt, string someString) => { /* Do something exciting! */ }, 
    option, argument);

The parameter names (someInt and someString) no longer need to match option or argument names. They're now bound to the options or arguments passed to CommandHandler.Create in the order in which they're provided to the SetHandler method. There are overloads supporting up to sixteen parameters, with both synchronous and asynchronous signatures.

As with the older CommandHandler.Create methods, there are various Task-returning Func overloads if you need to do asynchronous work. If you return a Task<int> from these handlers, it's used to set the process exit code. If you don't have asynchronous work to do, you can use the Action overloads. You can still set the process exit code with these by accepting a parameter of type InvocationContext and setting InvocationContext.ExitCode to the desired value. If you don't explicitly set it and your handler exits normally, then the exit code will be set to 0. If an exception is thrown, then the exit code will be set to 1.

Going to more than sixteen options or arguments using custom types

If you have a complex CLI, you might have more than sixteen options or arguments whose values need to find their way into your handler method. The new SetHandler API lets you specify a custom binder that can be used to combine multiple option or argument values into a more complex type and pass that into a single handler parameter. This can be done by creating a class derived from BinderBase<T>, where T is the type to construct based on command line input. You can also use this approach to support complex types.

Let's suppose you have a custom type that you want to use:

public class MyCustomType
{
    public int IntValue { get; set; }
    public string StringValue { get; set; }
}

With a custom binder, you can get your custom type passed to your handler the same way you get values for options and arguments:

var customBinder = new MyCustomBinder(stringArg, intOption);

command.SetHandler(
    (MyCustomType instance) => { /* Do something custom! */ },
    customBinder);

You can pass as many custom binder instances as you need and you can use them in combination with any number of Option<T> and Argument<T> instances.

Implementing a custom binder

Here's what the implementation for MyCustomBinder looks like:

public class MyCustomBinder : BinderBase<MyCustomType>
{
    private readonly Option<int> _intOption;
    private readonly Argument<string> _stringArg;

    public MyCustomBinder(Option<int> intOption, Argument<string> stringArg)
    {
        _intOption = intOption;
        _stringArg = stringArg;
    }

    protected override MyCustomType GetBoundValue(BindingContext bindingContext) =>
        new MyCustomType
        {
            IntValue = bindingContext.ParseResult.GetValueForOption(_intOption),
            StringValue = bindingContext.ParseResult.GetValueForArgument(_stringArg)
        };
}

The BindingContext also gives you access to a number of other objects, so this approach can be used to compose both parsed values and injected values in a single place.

Injecting System.CommandLine types

System.CommandLine allows you to use a few types in your handlers simply by adding parameters for them to your handler signature. The available types are:

  • CancellationToken
  • InvocationContext
  • ParseResult
  • IConsole
  • HelpBuilder
  • BindingContext

Consuming one or more of these types is straightforward with SetHandler. Here's an example using a few of them:

rootCommand.SetHandler(
    ( int i, 
      string s, 
      InvocationContext ctx, 
      HelpBuilder helpBuilder,
      CancellationToken cancellationToken ) => { /* Do something with dependencies! */ }, 
    option, argument);

When the handler is invoked, the current InvocationContext, HelpBuilder and CancellationToken instances will be passed.

Injecting custom dependencies

We've received a good number of questions about how to use dependency injection for custom types in command line apps built with System.CommandLine. The new custom binder support provides a simpler way to do this than was available in Beta 1.

There has been a very simplistic IServiceProvider built into the BindingContext for some time, but configuring it can be awkward. This is intentional. Unlike longer-lived web or GUI apps where a dependency injection container is typically configured once and the startup cost isn't paid on every user gesture, command line apps are often short-lived processes. This particularly important when System.CommandLine calculates tab completions. Also, when a command line app that has multiple subcommands is run, only one of those subcommands will be executed. If you configure dependencies for the ones that don't run, it's wasted work. For this reason, we've recommended handler-specific dependency configurations.

Putting that together with the SetHandler methods described above, you might have guessed the recommended approach to dependency injection in the latest version of System.CommandLine.

rootCommand.SetHandler(
    ( int i, 
      string s, 
      ILogger logger ) => { /* Do something with dependencies! */ }, 
    option, argument, new MyCustomBinder<ILogger>());

We'll leave the possible implementations of MyCustomBinder<ILogger> to you to explore. It will follow the same pattern as shown in the section Implementing a custom binder.

Customizing help

People ask very frequently how they can customize the help for their command line tools. Until a few months ago, the best answer we had was to implement your own IHelpBuilder and replace the default one using the CommandLineBuilder.UseHelpBuilder method. While this gave people complete control over the output, it made it awkward to reuse existing functionality such as column formatting, word wrapping, and usage diagrams. It's difficult to come up with an API that can address the myriad ways that people want to customize help. We realized early on that a templating engine might solve the problem more thoroughly, and that idea was the start of the System.CommandLine.Rendering experiment. Ultimately though, that approach was too complex.

After some rethinking, we think we've found a reasonable middle ground. It addresses the two most common needs that come up when customizing help and while also letting you use functionality from HelpBuilder that you don't want to have to reimplement. You can now customize help for a specific symbol and you can add or replace whole help sections.

The sample CLI for the help examples

Let's look at a small sample program.

var durationOption = new Option<int>(
    "--duration",
    description: "The duration of the beep measured in milliseconds",
    getDefaultValue: () => 1000);
var frequencyOption = new Option<int>(
    "--frequency",
    description: "The frequency of the beep, ranging from 37 to 32767 hertz",
    getDefaultValue: () => 4200);

var rootCommand = new RootCommand("beep")
{
    durationOption,
    frequencyOption
};

rootCommand.SetHandler(
    (int frequency, int duration) =>
    {
        Console.Beep(frequency, duration);
    },
    frequencyOption,
    durationOption);

When help is requested using the default configuration (e.g. by calling rootCommand.Invoke("-h")), the following output is produced:

Description:
  beep

Usage:
  beep [options]

Options:
  --duration <duration>    The duration of the beep measured in milliseconds [default: 1000]
  --frequency <frequency>  The frequency of the beep, ranging from 37 to 32767 hertz [default: 4200]
  --version                Show version information
  -?, -h, --help           Show help and usage information

Let's take a look at two common ways we might want to customize this help output.

Customizing help for a single option or argument

One common need is to replace the help for a specific option or argument. You can do this using HelpBuilder.CustomizeSymbol, which lets you customize any of three different parts of the typical help output: the first column text, the second column text, and the way a default value is described.

In our sample, the --duration option is pretty self-explanatory, but people might be less familiar with how the frequency range corresponds to common the common range of what people can hear. Let's customize the help output to be a bit more informative using HelpBuilder.CustomizeSymbol.

var parser = new CommandLineBuilder(rootCommand)
                .UseDefaults()
                .UseHelp(ctx =>
                {
                    ctx.HelpBuilder.CustomizeSymbol(frequencyOption,
                        firstColumnText: "--frequency <37 - 32767>\n             Bats:\n             Cats:\n             Dogs:\n             Humans:",
                        secondColumnText: "What frequency do you want your beep? For reference:\n   15 kHz to 90 kHz\n   500 Hz to 32 kHz\n   67 Hz to 45 kHz\n   20 to 20,000 Hz");
                })
                .Build();

parser.Invoke("-h");

Our program now produces the following help output:

Description:
  beep

Usage:
  beep [options]

Options:
  --duration <duration>     The duration of the beep measured in milliseconds [default: 1000]
  --frequency <37 - 32767>  What frequency do you want your beep? For reference:
               Bats:           15 kHz to 90 kHz
               Cats:           500 Hz to 32 kHz
               Dogs:           67 Hz to 45 kHz
               Humans:         20 to 20,000 Hz
  --version                 Show version information               
  -?, -h, --help            Show help and usage information

Only the output for the --frequency option was changed by this customization. It's also worth noting that the firstColumnText and secondColumnText support word wrapping within their columns.

This API can also be used for Command and Argument objects.

Adding or replacing help sections

Another thing that people have asked for is the ability to add or replace a whole section of the help output. Maybe the description section in the help output above needs to be a little flashier, like this:

  _                             _
 | |__     ___    ___   _ __   | |
 | '_ \   / _ \  / _ \ | '_ \  | |
 | |_) | |  __/ |  __/ | |_) | |_|
 |_.__/   \___|  \___| | .__/  (_)
                       |_|

Usage:
  beep [options]

Options:
  --duration <duration>     The duration of the beep measured in milliseconds [default: 1000]
  --frequency <37 - 32767>  What frequency do you want your beep? For reference:
               Bats:           15 kHz to 90 kHz
               Cats:           500 Hz to 32 kHz
               Dogs:           67 Hz to 45 kHz
               Humans:         20 to 20,000 Hz
  --version                 Show version information
  -?, -h, --help            Show help and usage information

You can change the layout by adding a call to HelpBuilder.CustomizeLayout in the lambda passed to the CommandLineBuilder.UseHelp method:

                    ctx.HelpBuilder.CustomizeLayout(
                        _ =>
                            HelpBuilder.Default
                                       .GetLayout()
                                       .Skip(1) /* Skip the boring default description section... */
                                       .Prepend(
                                           _ => Spectre.Console.AnsiConsole.Write(new FigletText("beep!"))
                                       ));

The HelpBuilder.Default class has a number of methods that allow you to reuse pieces of existing help formatting functionality and compose them into your custom help.

Making suggestions completions richer

One of the great things about System.CommandLine is that it provides tab completions by default. We’ve updated it to make more advanced scenarios easier.

If your users aren't getting tab completion, remind them to enable it by installing the dotnet-suggest tool and doing a one-time addition of the appropriate shim scripts in their shell profile. The details are here.

The completions model found in previous releases of System.CommandLine has provided tab completion output as a sequence of strings. This is the way that bash and PowerShell accept completions and it got the job done. But as we've started using System.CommandLine in richer applications such as for magic commands in .NET Interactive Notebooks, and as we've started looking at the capabilities of other shells, we realized this wasn't the most forward-looking design. We've replaced the string value for completions with the CompletionItem type, adopting a model that uses the same concepts as the Language Server Protocol used by many editors including Visual Studio Code.

In order to align to the naming found in common shell APIs and the Language Server Protocol, we've renamed the suggestion APIs to use the term "completion". However, the dotnet-suggest tool and the [suggest] directive that's sent to get completions from your System.CommandLine-powered apps have not been renamed, as this would be a breaking change for your end users.

Documentation

The public API for System.CommandLine now has XML documentation throughout, and we've started work on official online documentation and samples. If you find places where the XML documentation could be clearer or needs more details, please open an issue or, better yet, a pull request!

Deprecating System.CommandLine.Rendering

The System.CommandLine.Rendering project grew out of the realization that no help API could cover all of the ways in which someone might want to customize help. We started exploring approaches to rendering output that would look good in both older Windows command prompts that lack VT support as well as Linux and Mac terminals and the new Windows Terminal. This led to discussions about first-class support for VT codes and the lack of a separation of a terminal concept in System.Console. That discussion is ongoing and has implications well beyond the scope of this library. In the meantime, many of the objectives of System.CommandLine.Rendering were realized beautifully by the Spectre.Console project.

What about DragonFruit?

The System.CommandLine.DragonFruit library started as an experiment in what a simple-as-possible command line programming model could look like. It's been popular because of its simplicity. With C# now supporting top-level code, and with the arrival of source generators, we think we can simplify DragonFruit further while also filling in some of the gaps in its capabilities, such as its lack of support for subcommands. It's being worked on and we'll be ready to talk more about it after System.CommandLine reaches a stable 2.0 release.

The path to a stable 2.0 release

So what's next? When we've received enough feedback to feel confident about the latest changes, and depending on how much needs to be fixed, we'll publish an RC version or two. A stable release will follow. In the meantime, we're working on major performance improvements that will introduce a few additional breaking changes. The timeline will be driven by your feedback and our ability to respond to it.

This project has depended on community contributions of design ideas and code from the very beginning. No one has said we need to ship a stable version by any specific date. But it's our hope that we can be stable at last by March. You can help by downloading the latest version, upgrading your existing System.CommandLine apps or building some new ones, and letting us know what you think.

Thanks!

@atruskie
Copy link

atruskie commented Dec 18, 2021

Can you expand a little more on the deprecation of System.CommandLine.Rendering? I worked with it a bit, and while it had rough edges, I quite liked it's potential.

I was actually hoping Spectre.Console would build off the primitives provided by rendering (like TextSpan and even the live rendering things). This in my mind had a nice balance: common primitives would be provided by .NET, which community libraries, like Spectre.Console, can expand on.

I'd avoided Spectre.Console for a few projects because I didn't see it as building on a future part of .NET. (Don't get me wrong, Spectre.Console is great, it is just taking a different approach)

Would this be related to #902?

@tomrus88
Copy link

tomrus88 commented Dec 19, 2021

Why is new API requires so much boilerplate code?

Why can't new API instead of

// This is the same example as above.
var option = new Option<int>("-i");
var argument = new Argument<string>("-s");
var rootCommand = new RootCommand
{
    option,
    argument
};

// This is new!
rootCommand.SetHandler(
    (int someInt, string someString) => { /* Do something exciting! */ }, 
    option, argument);

be something like this

var rootCommand = new RootCommand<int, string>((int someInt, string someString) => { /* Do something exciting! */ })
{
    new Option<int>("-i"),
    new Argument<string>("-s")
};

?

This actually works if you write custom RootCommand class:

public class RootCommand<T1, T2> : RootCommand
{
    private Action<T1, T2> action;

    public RootCommand(Action<T1, T2> action, string description = "") : base(description)
    {
        this.action = action;
    }

    public void SetHandlerAndInvoke(string[] args)
    {
        var symbols = this.Children.Cast<IValueDescriptor>().ToArray();
        this.SetHandler(action, symbols);
        this.Invoke(args);
    }
}

Or another variant that accepts things though constructor instead of object initializer

var rootCommand = new RootCommand<int, string>((int someInt, string someString) => { /* Do something exciting! */ },
    "desc",
    new Option<int>("-i"),
    new Argument<string>("-s"));
public class RootCommand<T1, T2> : RootCommand
{
    public RootCommand(Action<T1, T2> action, string description = "", params IValueDescriptor[] symbols) : base(description)
    {
        var symbols2 = symbols.Cast<Symbol>();
        foreach (var symbol in symbols2)
            this.Add(symbol);
        this.SetHandler(action, symbols);
    }
}

@craignicol
Copy link

Thanks for the new CommandHandler syntax, it's definitely easier to follow. Does the new syntax support DateTime arguments though, or will I need to use the binding helpers for that, as I can't see dates in the examples?

Basically, the following code compiles, but fails at runtime for me:

               var dailyCmd = new Command("daily", description: "Generate Daily Statistics")
            {
                new Option<DateTime>("--start-date", () => DateTime.Now.AddDays(-1).Date, description: "Date to start reporting from (inclusive) - default: yesterday"),
                new Option<DateTime>("--end-date", () => DateTime.Now.AddDays(-1).Date, description: "Date to end reporting to (inclusive) - default: yesterday"),
                new Option<DirectoryInfo>("--output-folder", () => new DirectoryInfo(Directory.GetCurrentDirectory()), "Folder to output to. Will overwrite any existing files that match the given dates")
            };

                dailyCmd.SetHandler((DateTime startDate, DateTime endDate, DirectoryInfo outputFolder) => DailyStatsReport(startDate, endDate, outputFolder, services).Wait());

0xced added a commit to 0xced/CodeSign that referenced this issue Dec 20, 2021
I'm not familiar with the `System.CommandLine` API but it seems something has changed recently, see [Announcing System.CommandLine 2.0 Beta 2 and the road to GA][1]. I think this happened because the `System.CommandLine` package is referenced with a floating version (2.0.0-*) and a breaking change was introduced.

Anyway, `CommandHandler.Create` has been moved into a new `System.CommandLine.NamingConventionBinder` compatibility NuGet package and the new way of setting up a command handler is with a new `SetHandler` extension method.

[1]: dotnet/command-line-api#1537
@jonsequitur
Copy link
Contributor Author

Basically, the following code compiles, but fails at runtime for me:

@craignicol You'll need to pass the symbols you want to bind to SetHandler.

@craignicol
Copy link

Basically, the following code compiles, but fails at runtime for me:

@craignicol You'll need to pass the symbols you want to bind to SetHandler.

Ah, thanks. The error message in that situation was throwing me off.

@jonsequitur
Copy link
Contributor Author

We're going to improve this error message. This API should also be much easier than the old one to add analyzer support for.

filipnavara pushed a commit to filipnavara/CodeSign that referenced this issue Dec 20, 2021
I'm not familiar with the `System.CommandLine` API but it seems something has changed recently, see [Announcing System.CommandLine 2.0 Beta 2 and the road to GA][1]. I think this happened because the `System.CommandLine` package is referenced with a floating version (2.0.0-*) and a breaking change was introduced.

Anyway, `CommandHandler.Create` has been moved into a new `System.CommandLine.NamingConventionBinder` compatibility NuGet package and the new way of setting up a command handler is with a new `SetHandler` extension method.

[1]: dotnet/command-line-api#1537
@SamZhangQingChuan
Copy link
Contributor

Why can't new API instead of

// This is the same example as above.
var option = new Option<int>("-i");
var argument = new Argument<string>("-s");
var rootCommand = new RootCommand
{
    option,
    argument
};

// This is new!
rootCommand.SetHandler(
    (int someInt, string someString) => { /* Do something exciting! */ }, 
    option, argument);

be something like this

var rootCommand = new RootCommand<int, string>((int someInt, string someString) => { /* Do something exciting! */ })
{
    new Option<int>("-i"),
    new Argument<string>("-s")
};

?

This actually works with custom RootCommand class:

public class RootCommand<T1, T2> : RootCommand
{
    private Action<T1, T2> action;

    public RootCommand(Action<T1, T2> action, string description = "") : base(description)
    {
        this.action = action;
    }

    public void SetHandlerAndInvoke(string[] args)
    {
        var symbols = this.Children.Cast<IValueDescriptor>().ToArray();
        this.SetHandler(action, symbols);
        this.Invoke(args);
    }
}

Or another variant that accepts things though constructor instead of object initializer

var rootCommand = new RootCommand<int, string>((int someInt, string someString) => { /* Do something exciting! */ },
    "desc",
    new Option<int>("-i"),
    new Argument<string>("-s"));
public class RootCommand<T1, T2> : RootCommand
{
    public RootCommand(Action<T1, T2> action, string description = "", params IValueDescriptor[] symbols) : base(description)
    {
        var symbols2 = symbols.Cast<Symbol>();
        foreach (var symbol in symbols2)
            this.Add(symbol);
        this.SetHandler(action, symbols);
    }
}

I second @tomrus88. Writing each option/argument twice or even three times seems very redundant.

@ghord
Copy link

ghord commented Dec 31, 2021

Could we add something to avoid obvious boiler plate in this situation? It seems very logical to me to just use all arguments/options by default in the order of appearance in Children collection.

I now do this everywhere where I register command anyway:

var addDbCommand = new Command("add", "adds database aliases")
{
    new Argument<string>("alias", "database alias"),
    new Argument<string>("connectionString", "connection string to database"),
};

addDbCommand.SetHandler<string, string>(DatabaseAdd, addDbCommand.Children.OfType<IValueDescriptor>().ToArray());

@farlee2121
Copy link

I tried making a wrapper in F#. My goal was to have each level as a self-contained expression so that the different components can be laid out in a nice readable hierarchy, or declared in chunks and composed.

For example

let root =
       Cli.root "Example Description" [
           Cli.command "tag-extract" "Get content (list items, paragraphs, sections, etc) with the given tag" [
               Cli.argument<FileInfo> "input-file" "The file to extact content from"
               Cli.option<string> ["--tags"; "-t"] "One or more tags marking content to extract (e.g. 'BOOK:', 'TODO:')"
           ]
       ]

root.Invoke args

The key problem that I run into is the handler binding. The multi-arity overloads of SetHandler would require a separate wrapper for each overload, else the delegate and argument types can't be inferred correctly.

I'm struggling to find a good way around this though. There is no base class for Func and/or Action such that we could take any instance then try to pattern match on the number of arguments. Requiring a class with properties for expected inputs puts us back in the situation of implicit conventions.

@jonsequitur
Copy link
Contributor Author

@ghord

Could we add something to avoid obvious boiler plate in this situation? It seems very logical to me to just use all arguments/options by default in the order of appearance in Children collection.

We're working on an API layer to sit on top of this (a sort of successor to DragonFruit) that will reduce the boilerplate.

In the simple case, what you're proposing is intuitive and it would be simple enough for people to create wrappers that reduce the verbosity. It gets harder to make this work clearly and consistently when you have options or arguments that are reused in multiple commands, global options, options that are bound to a subcommand's handler but appear on a different command in the parser, or parsers that are composed programmatically (e.g. using attributes or source generators) where the order they occur in the collection isn't deterministic. We've erred on the side of specificity here to keep the parser and binder setups more decoupled.

@farlee2121
Copy link

farlee2121 commented Jan 3, 2022

My musings on dealing with variable arity reminded me the solution already exists in ICommandHandler.
This raised the question, why does parameter order get special treatment with SetHandler?

Why not create an API like

var durationOption = new Option<int>(...);
var frequencyOption = new Option<string>(...);

command.Handler = CommandHandler.FromValueOrder((int first, string second) => { ... }, durationOption, frequencyOption) 

Then the old convention-based factory could be aliased like

CommandHandler.FromParameterNameConvention<int, string>((i, s) => { });

This would be a fairly simple adaptation from the current command extensions.
Some benefits

  • Value-order has a consistent syntax with other handler strategies
  • Low signature conflict between strategies
  • It's easy to change the bundled handler factories
  • The order-based handler factory can be unbundled with a clear compatibility path
    • This is probably also true of the current extension-based approach
  • It's easier to wrap and extend the handler factory
  • There is more documentation about the binding strategy in the code
    • Pattern also suggests alternate binding strategies

Some downsides

  • Default path is perhaps not as discoverable

Thoughts?

@solvingj
Copy link

solvingj commented Jan 3, 2022

I agree with the observations that there seems to be a lot of boilerplate, although I see fundamental problems with all the suggestions. Sadly, I don't think you'll ever escape the very undesirable duplication and boilerplate people are complaining about with the current design of the API. There's a fundamental tension/struggle between the way this API is designed, and the goal of type safety.

My suggestion will perhaps be too radical and late to the party to be useful, but here it is for posterity.

With command parsing, we have the benefit of knowing and declaring the full structure of our command parser at compile time. This is a compile-time problem, however, the API's are built around the runtime-pattern of "construct-then-mutate". This will always be the fundamental source awkward struggles in the codebase and for the users. The workarounds will continue to be more and more complicated and exotic, and feel less and less maintainable. It's just the wrong pattern.

Consider the following alternative API, which resolves most of the awkward issues discussed previously (and others) by using the type system in a more powerful (and simpler) way. It emphasizes the virtue of "correct-by-construction".

public record BeepCommand : INewRootCommand
{
    public string Name() => "Beep Command";
    public string Description() => "Makes Beep Sounds.";
    public Duration Duration = new();
    public Frequency Frequency = new();
    public void Handle(BindingContext bc)
    {
        Console.Beep(Duration.value, Frequency.value);
    }
}

public record Duration : INewOption<int>
{
    public string Name() => "Duration";
    public string Description => "The duration of the beep measured in milliseconds";
    public int DefaultValue() => 1000;
}

public record Frequency : INewOption<int>
{
    public string Name() => "Frequency";
    public string Description() => "The frequency of the beep, ranging from 37 to 32767 hertz";
    public int DefaultValue() => 4200;
}

static int Main(string[] args)
{
    return BeepCommand().InvokeAsync(args).Result;
}

The benefits in this approach seem obvious and numerous to me, so I won't list them. However, it's worth pointing out that there are several tradeoffs (e.g. it inherently involves more class declarations). Perhaps this and other tradeoffs are simply non-starters for the maintainers.

Also, as I said, I recognize this might simply be too different from the current design, and require too much refactoring and redesign to be practical given the "near-release" stage of the project. However, I wanted to highlight that all the current thinking and discussion around these problems is trapped in a box: the "Construct-then-mutate" pattern. It is fundamentally the source of all the complicated overload resolution tricks, debates about convention rules, and duplicate declarations, and the sooner everyone understands that, the less people will bang their heads against the wall trying to find clever workarounds.

If I'm wrong, and the maintainers DO want to explore this further, I am willing to discuss and work on a PR with a POC.

In any case, for any readers passing by, please thumbs up or thumbs down based on if you think the API is better or worse respectively.

@jonsequitur
Copy link
Contributor Author

@solvingj Thanks for this example. This kind of API can absolutely be built on top of the existing library and we've done experiments along these lines from the outset to help guide the design. (DragonFruit is one.) Ultimately, these convention-based approaches make certain features easier to use and others harder. Our goals for the core System.CommandLine design are:

  • Don't limit the kinds of command line grammars the parser is capable of handling. (POSIX conventions, Windows conventions, aliases, subcommands, custom parsing.)
  • Provide a great experience for your end-users without you having to do all the work. (Tab completion, localizability, help, parse previews, and so on.)
  • Optimize for performance and trimmability.

I've found these are more objective and measurable than the ergonomics of the API. We've heard preferences including models like yours, attribute-annotated models, fluent interfaces, and DSLs. @farlee2121's comment above about F# ergonomics is in this category as well.

But if we've gotten the foundations right, then it's our belief that improved ergonomics can be achieved at another layer. Source generators are our bet for doing that without sacrificing performance. We've called this effort DragonFruit+, the design is at a much earlier stage, and we're very open to suggestions.

@solvingj
Copy link

solvingj commented Jan 4, 2022

@jonsequitur thanks for the prompt and thoughtful response.

I can understand that there are more factors and priorities to consider than most users will ever realize, especially on an important project like this one. I'm still personally unable to comprehend how the priorities and factors led to the current API, but I can honestly say "I've been there myself" on some past codebases, so I can relate. I tip my hat to you for creating this issue to gather feedback at this point. It's often difficult to take feedback at this stage.

With that said, I can only reiterate that I still believe you've got broken fundamentals in the core and there going to haunt the maintainers and the users for a long time if you don't take the time to rethink them now.

I also want to provide an alternative description of the fundamental problem. The API says to construct a command in one place, and construct it's handler functions separately, even though they are completely and utterly tightly coupled. There is a deep relationship between each Command and it's Handlers, and it's invariant in nature. They really REALLY should be captured together within classes. It's literally the textbook purpose of a class.

But we're currently not doing that, and as a result, we end up with two massive bundles of complexity to try to mitigate the problems:

  • The SetHandler method
  • Custom Binders

The SetHandler method is the source of VAST complexity, both internally for it's implementation, and for consumers to use correctly. It's complex because it's doing something really hard: providing a super generic function bridge function between the internals of the library and the users code.

But here's the most important point to understand about the SetHandler function: it literally does NOT need to exist. It's simply an extremely unfortunate consequence of the choice to allow people to construct handler functions outside of Command classes. This means both the classes and the call to SetHandler have to define all the same list of types for the options and arguments separately, and deal with a bunch of other challenges like parameter ordering and variable naming. The cost on the codebase is also massive, as the implementation to present this function is extremely complicated. SetHandler creates immense accidental complexity and provides no unique or intrinsic value: it does not need to exist.

The other untenable consequence of SetHandler is Custom Binders. This situation of having to refactor your entire parser into Custom Binders after your SetHandler function crosses the 16 parameter mark is shocking. Having two completely different solutions based on this arbitrary and low number of 16 just does not make sense. The ironic part is that I think this was an attempt to provide some better ergonomics for the simple case, despite the recent comments about ergonomics being a low priority. I'm guessing it's a compromise that ultimately didn't work out well.

It's worth noting that I think "Custom Binders" represent a more appropriate solution to the problem than the SetHandler method, but their implementation has to deal with a bunch of side-effects of the SetHandler function, so they're complicated to the point where they feel really verbose and hard to use. I really think a different solution is needed at this level.

In closing, I want to say that I really appreciate and respect all the hard work that's been done here. There's a TON of internals and features in this library which are really great and done right, and I'm excited to use them.

@farlee2121
Copy link

farlee2121 commented Jan 4, 2022

@solvingj I agree that SetHandler is complex to extend, but I think it's extreme to say the API is fundamentally broken.
The root of extensibility is not SetHandler but ICommandHandler. Your approach could be built as an alternative to SetHandler and plugged in nicely because of ICommandHandler.

This is why my last comment debated if the set handler should be brought more in line with the general handler factory pattern. It clarifies the substitution of ICommandHandlers.


New idea:
As for an API that would be easy to build on, I've been contemplating how to build a property-map-based handler factory.
For example

var durationOption = new Option<int>(new []{ "-d", "--duration"}, ... );
var frequencyOption = new Option<int>(new [] {"-f", "--frequency"}, ...);

command.Handler = CommandHandler.FromPropertyMap<CustomInputClass>(
  (CustomInputClass inputs) => { ... }, new []{
     PropertyMap.FromName("-f", (inputClass) => inputClass.Frequency)),
     PropertyMap.FromReference(durationOption, (inputClass) => inputClass.Duration))
  }
)

class CustomInputClass{
    public int Frequency {get; set;}
    public int Duration {get; set;}
}

This takes some inspiration from AutoMapper. A few benefits

  • Requiring a complex type allows us to do away with arity overloads
  • We can keep our data models separated from binding configuration
  • PropertyMap allows us access to all the generic type data we need while producing objects of uniform type that can be passed around, inspected, modified, etc
    • this also allows us freedom to mix property mapping strategies (e.g. some explicit, some convention)
  • This model can handle options shared between commands
  • This model can handle Input of arbitrary depth and complexity

@jonsequitur
Copy link
Contributor Author

@solvingj I'd like to problematize something you wrote:

The API says to construct a command in one place, and construct it's handler functions separately, even though they are completely and utterly tightly coupled. There is a deep relationship between each Command and it's Handlers, and it's invariant in nature. They really REALLY should be captured together within classes. It's literally the textbook purpose of a class.

I agree this is sometimes true. I disagree that it's always true. For example:

  • Not all commands have handlers.
  • The relationship of command to handler isn't always invariant in nature. (DragonFruit constructs its parser dynamically based on your Main method signature, and we're working on creating parsers dynamically for dotnet new.)
  • Class declarations aren't always the best approach. (They wouldn't be idiomatic in F#, and they were considered too much boilerplate and discarded for ASP.NET 6's minimal API.)

On this though I agree entirely:

But here's the most important point to understand about the SetHandler function: it literally does NOT need to exist.

This is precisely the point of ICommandHandler, as @farlee2121 identified:

Your approach could be built as an alternative to SetHandler and plugged in nicely because of ICommandHandler.

The ICommandHandler interface is very simple and it provides a lot of leeway in how you implement it. Want to make a class that inherits it and also implements Command? It works. Want to generate it dynamically using a source generator driven by your own coding conventions? We're working on such an implementation.

The point being that SetHandler is not in any way required. It's a convenience, and more ergonomic (subjective as that is) alternatives to creating handlers will likely come along shortly from Microsoft and from the OSS community. The decoupled nature of the current API is meant to enable that flexibility.

@tompazourek
Copy link

I don't see any news or mentions of System.CommandLine.Hosting.

If you don't mind me asking: Is System.CommandLine.Hosting still going to be developed looking forward? I find it tremendously useful, and it would be really great to see a stable release some day.

adrianbanks added a commit to adrianbanks/GameOfLife that referenced this issue Jan 5, 2022
@farlee2121
Copy link

I decided to try building my previous proposal, and it came together pretty smoothly. The repo is System.CommandLine.PropertyMapBinder.

Here's a sample registration

rootCommand.Handler = CommandHandler.FromPropertyMap(SuchHandler,
    new BinderPipeline<SuchInput>()
    .MapFromName("print-me", model => model.PrintMe)
    .MapFromReference(frequencyOpt, model => model.Frequency)
    .MapFromName("-l", model => model.SuchList)
);

It also handles mixed binding strategies well

rootCommand.Handler = CommandHandler.FromPropertyMap(SuchHandler,
    new BinderPipeline<SuchInput>()
    .MapFromNameConvention(TextCase.Pascal)
    .MapFromName("-l", model => model.SuchList)
);

I only implemented the three approaches, but here are a few more that would be useful and fairly easy to implement

  • map default values from configuration
  • Ask a user for any missing inputs
    • can be done with the existing setter overload, but prompts could be automated with a signature like .PromptIfMissing(name, selector)
  • match properties based on type
  • Set a value directly
    • can be done with the existing setter overload, but could be simpler .MapFromValue(c => c.Frequency, 5)

@Balkoth
Copy link

Balkoth commented Apr 21, 2022

As with the older CommandHandler.Create methods, there are various Task-returning Func overloads if you need to do asynchronous work. If you return a Task from these handlers, it's used to set the process exit code. If you don't have asynchronous work to do, you can use the Action overloads. You can still set the process exit code with these by accepting a parameter of type InvocationContext and setting InvocationContext.ExitCode to the desired value. If you don't explicitly set it and your handler exits normally, then the exit code will be set to 0. If an exception is thrown, then the exit code will be set to 1.

This is a joke right? I have non-asynchronous work to do and have to handle some weird InvocationContext to set the exit code? With no sample on how to do that? Why weren't there all scenarios that have worked previously ported to the new way of doing things?

@jonsequitur
Copy link
Contributor Author

The official documentation for setting an exit code includes a sample for using a return value in a non-async (but Task-returning) handler. Does this help? https://docs.microsoft.com/en-us/dotnet/standard/commandline/model-binding#set-exit-codes.

We're not satisfied with the large number of overloads but adding 16 more to avoid having to use Task.FromResult doesn't seem like an improvement either. The list in IntelliSense is already too crowded. We had to make a change here because CommandHandler.Create relies on reflection and is inherently untrimmable and less performant. But CommandHandler.Create is still available in System.CommandLine.NamingConventionBinder if you prefer the old API and don't plan to trim your app.

@Balkoth
Copy link

Balkoth commented Apr 25, 2022

I think it's a good thing that apps using this can now be trimmed, so i am all onboard with changes for the better. But instead of the library providing the overrides, you made every developer who is not doing async work in the handler, write more code themselves. Now there is not one way of returning the result from the handler, but two and you have to know when to use which. I don't think this is a good change.

@Balkoth
Copy link

Balkoth commented Apr 28, 2022

I think the way the options are bound to the handler and the command by default is bad. Please provide something like i do it below to set them all at once.

internal static class Program
{
  private static int Main(string[] args)
  {
    // Create options for the export command.
    Option[] exportOptions = new Option[]
    {
      new Option<bool>("--reports", "Exports reports."),
      new Option<bool>("--files", "Exports files.")
    };

    // Create options for the check command.
    Option[] checkOptions = new Option[]
    {
      new Option<bool>("--reports", "Checks reports."),
      new Option<bool>("--files", "Checks files.")
    };

    // Create a root command containing the sub commands.
    RootCommand rootCommand = new RootCommand
    {
      new Command("export", "Writes files containing specified export options.").WithHandler<bool, bool>(Program.HandleExport, exportOptions),
      new Command("check", "Checks the specified options.").WithHandler<bool, bool>(Program.HandleCheck, checkOptions)
    };

    return rootCommand.Invoke(args);
  }

  private static Task<int> HandleExport(bool reports, bool files)
  {
    return Task.FromResult(0);
  }

  private static Task<int> HandleCheck(bool reports, bool files)
  {
    return Task.FromResult(0);
  }
}

internal static class Extensions
{
  public static Command WithHandler<T1, T2>(this Command command, Func<T1, T2, Task> handler, Option[] options)
  {
    options.ForEach(option => command.AddOption(option));
    command.SetHandler(handler, options);
    return command;
  }
}

@farlee2121
Copy link

Adding alternative binding approaches is actually pretty easy through ICommandHandler.
I made an alternative approach based on the builder pattern and published it as a Nuget package. You could contribute your approach too.

@jonsequitur
Copy link
Contributor Author

@Balkoth SetHandler needs improvement and was the source of a lot of discussion in today's API review. We've looked at ways to make a more concise API like your WithHandler example without limiting the flexibility of the core API. The best solutions involve either reflection (which was moved out to System.CommandLine.NamingConventionBinder to support trimming of System.CommandLine) or source generation (which we're working on.) There was debate today about whether "syntactic sugar" methods to bind handlers to parsers belong in the core System.CommandLine at all.

@Balkoth
Copy link

Balkoth commented Apr 28, 2022

What i am doing in my sample is imho pretty basic stuff when working with System.CommandLine. Why should basic stuff not be approachable (you call it syntactic sugar) from there?

@jonsequitur
Copy link
Contributor Author

It absolutely should be approachable! The question (and it's under active debate) is whether the "approachable" parts (which are often a higher level of abstraction) belong in the base layer, with all of the long-term support and stability requirements that that entails.

@Balkoth
Copy link

Balkoth commented Apr 28, 2022

I understand that this may be a hot topic. My preference would be to keep this all in a single package.

@jonsequitur
Copy link
Contributor Author

It's a tricky design problem to be sure.

The base layer can't address everything for everyone, so our goals are to make it useful by itself, but in the case where it doesn't address people's needs, make it easy to build on top of rather than have to start from scratch.

@zivkan
Copy link
Member

zivkan commented Jun 2, 2022

lol, as I was typing this out, the beta 4 announcement was published. I need to focus on other things, so I can't try out beta 4 right now.

Please give the latest packages a try and let us know your thoughts.

For what it's worth, this week I updated to a beta 3version of the package. One problem I encountered and took me many hours to solve was trying to figure out how to do Dependency Injection in my app. I can't find it now, but in some existing issue, someone had written something like, people have two different use cases for DI. 1. discover all commands 2. modify DI registration based on option values. For better or worse, I thought I wanted to do both, but only the second was really importent.

This announcement however said the following:

command line apps are often short-lived processes. This particularly important when System.CommandLine calculates tab completions. Also, when a command line app that has multiple subcommands is run, only one of those subcommands will be executed. If you configure dependencies for the ones that don't run, it's wasted work. For this reason, we've recommended handler-specific dependency configurations.

I found this justification compelling, so I gave up on trying to use DI to discover all commands. I'm now using reflection to find all types that implement a specific interface, which might not be a whole lot better, but it avoids needing to register services for subcommands that won't run. Additionally, the example below the quote, that suggests doing DI registration and usage in the handler, that helped me solve the other problem.

I'm awful at documentation, so I don't have any suggestions on how to make it better. The above information about why not to use DI before a command handler, and how to use DI inside the command handler, that doesn't work well in reference docs, like in docs.microsoft.com/dotnet/api, but it really helped unblock me, so I think it's valuable to have easy to discover.

Anyway, my only feedback is that this works well for me. Thanks! 👍

@solvingj
Copy link

solvingj commented Jun 2, 2022

@zivkan

I'm now using reflection to find all types that implement a specific interface

If you think you might possibly want to use dotnet's AOT features to make your CLI apps "single-file-executables" (which many people are planning to do once the AOT stuff becomes GA), you may want to avoid using reflection because it is "AOT Unfriendly". There are comments about Reflection and AOT in these posts.
https://devblogs.microsoft.com/dotnet/announcing-dotnet-7-preview-3/
https://twitter.com/davidfowl/status/1391580410119819265?lang=en

@zivkan
Copy link
Member

zivkan commented Jun 2, 2022

Thanks @solvingj. I was using System.CommandLine for a team internal reporting tool (find all open github issues with a specific label, and auto-generate a markdown file, once rendered, can be copy-pasted to an email). So, zero chance of needing AOT for this specific tool. Plus, I don't believe that Assembly.GetTypes() needs Emit, so might be AOT compatible.

I had a glance over the blog post, but I didn't see what the recommendation is to avoid reflection. I'm assuming it's Source Generators. I probably spent 10 hours a few months ago trying to understand Roslyn's APIs for analyzers and source generators when I had a use-case for it, but unfortunately it just doesn't click with my brain. Plus, referencing analyzers/source generators with project references vs needing to create a package, publish it, update package reference version. I love the idea of analyzers and source generators, but for what I work on, it's just not worth the effort until it's easier. Maybe one day I'll find a scenario that is compelling enough to dedicate more effort to actually learning. Anyway, my lack of understanding of Roslyn APIs is way off-topic for this System.CommandLine announcement. Good advice though.

@jonsequitur
Copy link
Contributor Author

I had a glance over the blog post, but I didn't see what the recommendation is to avoid reflection.

Using the System.CommandLine APIs directly is Native AOT-friendly.

And yes, for more convention-based approaches where people have traditionally used reflection, source generators are a great option, and a library that layers over System.CommandLine and uses source generators is actually in the works.

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

No branches or pull requests