search

dotnet / command-line-api

Command line parsing, invocation, and rendering of terminal output.
https://github.com/dotnet/command-line-api/wiki
MIT License
3.34k stars 375 forks source link

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

Open jonsequitur opened 2 years ago

jonsequitur commented 2 years ago

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:

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:

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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());
jonsequitur commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

SamZhangQingChuan commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

Some downsides

Thoughts?

solvingj commented 2 years ago

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 commented 2 years ago

@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:

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 commented 2 years ago

@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 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 commented 2 years ago

@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

jonsequitur commented 2 years ago

@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:

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 commented 2 years ago

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.

farlee2121 commented 2 years ago

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

KathleenDollard commented 2 years ago

@farlee2121 That looks cool!

ptr727 commented 2 years ago

Hi, with b2 a list of strings no longer works, only the first string is recognized, remainder is report as invalid argument:

        private static Option CreateMediaFilesOption()
        {
            // Media files or folders option
            return new Option<List<string>>("--mediafiles")
            {
                Description = "List of media files or folders",
                IsRequired = true
            };
        }

Full code is here: https://github.com/ptr727/PlexCleaner/blob/main/PlexCleaner/CommandLineOptions.cs

How do I restore the ability to have multiple strings following an option?

jonsequitur commented 2 years ago

@ptr727, the issue might be this change to the default for Option.AllowMultipleArgumentsPerToken: https://github.com/dotnet/command-line-api/issues/1552#issuecomment-1004318166

ptr727 commented 2 years ago

Yep, seems to be the case. This is a breaking change, and unexpected.

Whenever an option is a list/array/iterator type, it is always expected that multiple values follow the argument. I think the behavior needs to be dynamic when using --arg item item item, vs. explicit when using --arg item --arg item --arg item.

solvingj commented 2 years ago

@jonsequitur I can appreciate the need for dynamic/runtime-evaluated command structure, and that the current design gives flexibility for other higher level API's (at the cost of optimizing for ergonomics). I have thought more on your other points and updated my perspective. I was suggesting to drop the add_handler strategy and thus avoid all the overload and other design quandaries it creates. I will leave that to you now as "implementation details".

My remaining fundamental concern is that there is not yet a reasonable "natively supported" alternative API to using add_handler strategy within this project. You've explained the key design philosophy, priorities, and goals in this thread and users should be aware of that. So, please consider explaining the context on the README. Something about: "This library intends to provide a low-level framework for consumption in other high-level opinionated/optimized/ergonomic CLI parsing libraries built on top of it, and not for direct consumption in CLI applications." It's kind of an exceptional (and I think wise) thing you're doing... most CLI libraries are highly opinionated and are positioned for end-user consumption.

I worry that the existing user base and new users coming to v2.0 will miss all of that context. They will see two very foreign looking user-experience choices (as I did): 1.) The Command() + add_handler strategy, and the radical Dragonfruit strategy. Despite them each having issues, many people will choose and implement one of them because it's the officially supported dotnet solution. Most users aren't experts in CLI API's, they'll just accept whatever examples and docs this library provides as "the new idiomatic patterns for creating CLI's in C#" They will then go on to copy and modify the examples to fit their application needs in large numbers, and have the sub-optimal experience we are all here discussing.

To avoid this, before GA, I would request that the core maintainers of this library choose, implement, and support at least one higher-level API/strategy on top of the existing framework. I suggest it be a "traditional CLI experience" (subjective I know) and optimized for ergonomics for some notion of "a common case" (again, subjective I know). The most important idea in this suggestion is that this project doesn't unintentionally encourage a ton of other projects to go off and write or re-write CLI's using add_handler or and a bunch of lambda's with type parameters everywhere. I won't even suggest a preference between fluent, annotation based, interface based, or convention based... those are all somewhat "traditional". Just please don't leave add_handler and "dragonfruit 2.0" as the only natively-supported options.

Also, it's an important point that such an "official" user-oriented-API should be maintained under dotnet (and probably in this repo/project just like dragonfruit was). The tradeoffs for adding a third party dependency to get a "ergonomic API" will likely result in many teams opting to use this project vanilla, and then dealing with add_handler as an unfortunate side-effect.

As a sidenote, Dragonfruit is a very novel and interesting concept and experiment, and indeed radical. I love doing this kind of work myself. With that said, rather than putting time into a "successor" to something so radical, I suggest (and selfishly request), that you all spend time providing a "traditional", "intuitive", "type-safe", and "ergonomic" API layer instead (before 2.0).

Thanks again for all your work and I apologize if my comments appear negative. In any case, I hope you can find some merit and value in them.

farlee2121 commented 2 years ago

@solvingj I'd be interested in how ergonomic and extensible you find my PropertyMapBinder.

Also, from my experience building the above, I found ICommandHandler pretty easy to work with. I definitely think the average user could tackle it with a smidge of showcased documentation. It's really anything that takes an InvocationContext and returns an exit code. All the difficult parsing and what not is already done and available through the context like context.ParseResult.GetValueForOption(option)

solvingj commented 2 years ago

@farlee2121 it looks nice. Well done. It emphasizes and uses modern patterns and strategies like composable pipelines and builder strategies which is often good, but I've personally never needed or wanted to use those for CLI's. That said, I'm certainly willing to believe there are some use-cases and users who might prefer them.

But I guess this is the nice thing about the core library being low-level. Different audiences can implement different experiences without having to do the hard parts you mentioned (parsing and whatnot).

I will tell you more specifically that the fundamental reasons I would not use this strategy. One, I find fall-through behavior like pipelines powerful and flexible, but harder to reason about and maintain. Sometimes it's even less deterministic. I generally want minimal indirection and simpler maintenance in my CLI parsers, with maximum determinism. Power and flexibility aren't generally priorities.

The last (and biggest) reason is the same as the reason I gave for not wanting to use the add_handler parser, which I explained before:

public class SuchInput {
    public int Frequency { get; set; }
    public string? PrintMe { get; set; }
    public IEnumerable<int> SuchList { get; set; } = Enumerable.Empty<int>();
}

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 is fundamentally separating declaration and implementation details of each option/setting into different classes. This means that invariants for each option/argument are defined in an ad-hoc way, out in some generic/main business logic/body/control flow of the application which is separate from the declaration of the option/argument itself:

.MapFromReference(frequencyOpt, model => model.Frequency)

Is completely disconnected from this:

public int Frequency { get; set; }

When you look at the SuchInput class, and the Frequency member, there is no indication of what rules there are around it (what invariants might be applied). Is there a min/max allowable value? Does it have some relationship with other arguments? The only way to tell is to find the place in the application where an instance of BinderPipeline is constructed, and that property is mapped. It's unnecessarily awkward, and causes loads of redundant/unnecessarily/duplicate type and variable declarations. The pattern also opens the door to multiple codepaths leading to different places where BinderPipeline might be constructed, and might use different mapping functions. This sounds like flexibility, but is rarely needed in CLI parsers (compared to the common case). All of these are non-problems when command-line options are declared in a class, and getters/setters in that class express relationships and invariants with other options/args/etc.

Again, I'm describing the most straightforward approach to deal with the most straightforward requirements and cases for CLI applications. There are valid reasons to approach the problem differently and separate argument declarations from their implementation details, but for me, those reasons would have to be very compelling. Your use cases might have such reasons.

Also, just a nitpick for future reference, but when calling FromPropertyMap() above, you're passing a BinderPipeline which you are constructing in-line. I guess you like that it saves lines and avoids a temporary, which makes the code "terse", which can seem good for an example. But the constructor is 4 lines long and each line is non-trivial with a lambda. As far as examples go, IMO, this pattern makes it look more complex than it really is. I would have preferred the explicit temporary.

farlee2121 commented 2 years ago

Hmm, I agree invariants are being separated from the data type, but that's intentional. I like to keep my transfer objects separate and perform validation at the domain layer boundary. Then my domain doesn't depend on client concerns. I also have my domain rules in one place and can fully model different failure modes as part of the domain model (I highly recommend Domain Modeling Made Functional if the idea piques your interest).

I'd gladly give your approach a go if you decide to implement it. I'm always up for learning from different approaches.

SuperJMN commented 2 years ago

I somewhat got the the last part of the discussion. And I still don't know how to deal with this:

new Argument<SomeComplexType>(...)

Can somebody tell me how to map from a string coming from an option/argument to another class? (for instance, an argument that takes an string and maps it to a IFileInfo (that is a rather common scenario).

Thanks in advance!

xEsteem commented 2 years ago 
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 to you to explore.

Can someone help me understand here as I think this is the bit I understand least and the docs at the top forego an example. If I have a service that requires other services as part of its instantiation, how does this work? BinderBase doesnt seem to have any IServiceProvider to use here, nor can i think of a way to use a host, using system.commandline.hosting, within a BinderBase. I guess at this point one option is to spin up the service collection before creating the root command

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

?? but this seems redundant and also undoes the reason that this change was introduced anyway as you still need to configure services up front

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. Can someone highlight a good practice of creating a binder to pass into the SetHandler method for a service that might take a couple other services in it's constructor, that might also need to be used elsewhere to be passed into other services or something. I'm not fully understanding the expected practice here for the dependency injection.

solvingj commented 2 years ago

@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.

@jonsequitur if you recall, the other issue I commented on recently was that the following PR effectively closes off numerous different approaches for wrapping and extending new API's (including the example I gave).

https://github.com/dotnet/command-line-api/pull/1538

All statements below are "IMO", one persons opinion: It was wise and correct from a domain modeling perspective to make these things interfaces: (ICommand, IArgument, and IOption etc). It was narrow-focused to fundamentally compromise the domain model for a truly negligible performance increase. This isn't the hot path of a 10 GB network driver, this is a command-line parser. That's not to say performance isn't important, but it's not so important we need to make "any and all possible sacrifices for any possible performance gain". The number of types the compiler needs to JIT should not be dictating the model. It should not even be in the conversation. This is the definition of a premature optimization.

IMO, that PR is a mistake with massive implications on the future and should be reverted.

jonsequitur commented 2 years ago

It was wise and correct from a domain modeling perspective to make these things interfaces: (ICommand, IArgument, and IOption etc). It was narrow-focused to fundamentally compromise the domain model for a truly negligible performance increase.

@solvingj Performance wasn't the motivator for removing these, it was just a (very small) bonus. It was under discussion long before we started this performance work. While people could have implemented ICommand, etc. rather than inheriting Command, it's very unlikely the parser would work as expected because many code paths were casting to assumed singular concrete implementations. I have no idea what the expected behavior should be if someone implemented both ICommand and IArgument on the same class, for example, but I'm certain it wouldn't be what anyone intended. (I'm not aware of anyone having filed a bug related to implementing these interfaces and I know it wouldn't work well if they tried. This is evidence that it might not have been that interesting as an extension point.) So this abstraction was never completely implemented and to implement it fully would have rendered the difference between the symbol interfaces and their implementations meaningless. Meanwhile the extra abstraction had a cost in complexity that went beyond the JIT cost for the extra types. It led to confusion about which types to use when and it increased the cyclomatic complexity and maintenance cost of the code. The easier extension point has always been inheritance of the concrete types.

jonsequitur commented 2 years ago

Thanks for all of the great feedback and bug reports!

Beta 3 is available now. You can read the announcement here: #1613.

Exeteres commented 2 years ago

For those who are looking for an alternative command syntax 👀

I wrote a wrapper library over System.CommandLine that allows you to declare commands like controllers in ASP.NET. At the moment, some features and documentation are missing here, but this approach itself may be of interest to someone.

P.S. It's sad that System.CommandLine.Rendering is now deprecated. It would be very convenient to use it with controllers.

Something like: ```c# [CommandGroup("repositories")] public class RepositoryController : Controller { private readonly IRepositoryManager _repositoryManager; public RepositoryController(IRepositoryManager repositoryManager) { _repositoryManager = repositoryManager; } [Command("list", Description = "List all added repositories")] public async Task ListRepositories() { var repositories = await _repositoryManager.GetAllAsync(CancellationToken); var table = new TableView { Items = repositories }; table.AddColumn(x => x.Id, "Id"); table.AddColumn(x => x.DisplayName, "Name"); return new StackLayoutView { table }; } } ```
Balkoth commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

jonsequitur commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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.