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

Proposal: CLI Actions #2071

Open
jonsequitur opened this issue Mar 1, 2023 · 11 comments
Open

Proposal: CLI Actions #2071

jonsequitur opened this issue Mar 1, 2023 · 11 comments
Labels
Area-API needs discussion Further discussion required

Comments

@jonsequitur
Copy link
Contributor

jonsequitur commented Mar 1, 2023

This proposal describes a rethinking of the approach to command invocation that's been in place in System.CommandLine from the beginning. The convention- and reflection-based approach (CommandHandler.Create(Func<...>) and overloads) was removed in past previews in favor of something suited to better performance, trimming, and NativeAOT compilation (Command.SetHandler(Func<...>) and overloads). This change reduced the usability of the API by increasing its verbosity. The current proposal is for a design to replace these APIs with something quite different. (For more background and discussion, see #1776.)

A few examples

I'll start with a few code examples before discussing the various specific goals of the design.

The core concept adds a new abstraction, roughly similar to the existing ICommandHandler, called a CliAction. The core of the interface looks like this:

public abstract class CliAction
{
    public abstract Task<int> RunAsync(
        ParseResult parseResult, 
        CancellationToken cancellationToken); 
}

(The parameters to RunAsync assume that the InvocationContext class will be combined into ParseResult, a change currently under discussion.)

A CliAction can be attached to a command (or other symbol--more on that below) very much like a CommandHandler today:

var rootCommand = new RootCommand();
rootCommand.SetAction(new MyRootCommandAction())

For convenience, the action can be set up using a delegate, much like today:

rootCommand.SetAction((parseResult, cancellationToken) => 
{
    /* custom logic */ 
});

After parsing, you can run the action designated by the command line input like this:

ParseResult result = rootCommand.Parse(args);
await result.Action.RunAsync();

Just like the existing InvokeAsync APIs, this will run the code associated with the command the end user specified.

While there are a number of similarities to the current command handler API, it's intended to be used somewhat differently, using different CliAction types to represent outcomes that you can inspect and alter prior to invocation.

The key difference can be summed up as:

The ParseResult.Action property, set during parsing, makes the behavior designated by the parsed command line explicit, inspectable, and configurable prior to invocation.

Goals:

Clearly separate parsing from invocation.

The Parse and Invoke methods currently appear as sibling methods on Parser and Command, which, while convenient, has led many people to misunderstand that Parse is a prerequisite to Invoke. The Invoke methods internally call Parse. It seems helpful to separate these.

Reduce the number of overloads associated with setting up handlers.

By associating command line options and arguments directly with handler method parameters, the existing API makes it looks like you need one handler parameter for each command line parameter. For example, a command line like --one 1 --two 2 --three 3 should map to a handler method such as DoSomething(int one, int two, int three). This led to a huge number of overloads accepting from 1 to 8 parameters with variations returning void, int, Task, or Task<int>. It's confusing, especially when you need more than 8 parameters. (The answer we provided, using BinderBase<T>, isn't discoverable or intuitive.)

We'll be pulling out most of these overloads in favor of just the ones that accept ParseResult (replacing InvocationContext in the handler APIs). The result is that the API usage will be the same whether you need one parameter or fifty. In combination with slightly reducing verbosity by reintroducing name-based lookups, the API should be simpler to use. Here’s a comparison:

Current:

var fileOption = new Option<FileInfo>("--from");
var destinationOption = new Option<DirectoryInfo>("--to");
var rootCommand = new RootCommand()
{
    fileOption,
    destinationOption   
};
rootCommand.SetHandler((from, to) => 
{
    // logic to move the file
}, fileOption, destinationOption);

Proposed:

var rootCommand = new RootCommand()
{
    new Option<FileInfo>("--from"),
    new Option<DirectoryInfo>("--to")
};
rootCommand.SetAction(parseResult => 
{
    var from = parseResult.GetValue<FileInfo>("--from");
    var to = parseResult.GetValue<DirectoryInfo>("--to");
    // logic to move the file
});

Provide better support for building source generators.

Working on a source generator for System.CommandLine exposed a number of places where the current configuration API is difficult to use. The middleware pipeline, while flexible, is also not inspectable, making it hard for API users to see what's already been configured. Configurations of unrelated subsystems are unnecessarily coupled via the CommandLineBuilder.

We plan to make the CommandLineConfiguration class mutable and hopefully remove the CommandLineBuilder altogether (since the motivation for it was to support building immutable configuration objects). (See below for how behaviors will be configurable under this proposal.)

Broaden the concept of which symbol types support invocation to include Option and Directive symbols.

Commands have always supported invocation. They are the most common indicators of different command line behaviors, while options usually represent parameters to those behaviors. But that's not always the case. Sometimes options have behaviors. The help option (-h) is the most familiar example. Then there are directives, a System.CommandLine concept, which also have behaviors that can override the parsed command. We realized that allowing other symbol types to have behaviors, and using a common abstraction for all of these, is clearer than having to implement the option and directive behaviors through middleware.

The result is that in the proposed API, a CliAction can be attached to any of these types.

(An alternative proposal that's been raised is to handle help by making -h a command. But -h really does act grammatically as an option, not a command, and enabling this would require other fundamental and likely confusing changes to syntax concepts that have been stable and well-understood so far.)

Remove execution concepts from configuration.

The CommandLineConfiguration type actually configures two different aspects of your CLI app:

  • the parser rules that define the CLI's syntax and grammar (e.g. Command, Option<T>, Argument<T>, POSIX rules, etc), and

  • the behaviors of the CLI (exception handling, help formats, completion behaviors, typo correction rules).

This proposal would remove the latter entirely from CommandLineConfiguration. Behavioral configuration would be available on specific CliAction-derived types.

Make configuration of CLI actions lazy and make associated configuration APIs more cohesive with the handler code.

A common style of CLI app performs one of a set of mutually exclusive actions (subcommands or verbs) and then exits. We've recommended configuring the behaviors of those actions lazily because, since most won't be needed most of the time, it will generally give better startup performance. (We've often made the recommendation not to configure DI globally for CLI apps for this same reason.)

The CommandLineBuilder API, though, lends itself to configuring all of these behaviors centrally. This reduces the cohesiveness of the code associated with a specific behavior.

Help is a good example to illustrate this.

In the current help API, you might do something like this using the CommandLineBuilder and HelpBuilder to add some text after the default help output:

var rootCommand = new RootCommand();
Parser parser = new CommandLineBuilder(rootCommand)
    .UseDefaults()
    .UseHelpBuilder(helpContext => 
    {
        helpContext.HelpBuilder.CustomizeLayout(CustomLayout);

        IEnumerable<Action<HelpContext>> CustomLayout(HelpContext _)
        {
            foreach (var section in HelpBuilder.Default.GetLayout())
            {
                yield return section;
            }

            yield return ctx => ctx.Output.WriteLine("For more help see https://github.com/dotnet/command-line-api");
        }
    })
    .Build();

Under this proposal, the code would now look like this:

// setup:
var rootCommand = new RootCommand();
var helpOption = new HelpOption();
rootCommand.Add(helpOption);
helpOption.SetAction(new HelpAction());

// usage:
var result = rootCommand.Parse(args);

switch (result.Action)
{
    case HelpAction helpAction:
        await helpAction.RunAsync();

        Console.WriteLine("For more help see https://github.com/dotnet/command-line-api");
        break;

    default:
        await result.Action.RunAsync();
        break; 
}

Other built-in CliAction types for existing behavior might include ParseErrorAction, CompletionAction, and DisplayVersionAction. Commands could opt to use custom action types or use a default CommandAction.

Replacing existing functionality outright with custom implementations would also be much simpler under the proposed API:

// setup:
var helpOption = new HelpOption();
helpOption.SetAction(new SpectreHelpAction());

This approach removes abstractions and reduces the concept count relative to the existing builder pattern. The use of pattern matching allows a clearer and more cohesive way to intercept, augment, or replace existing functionality.

Make "pass-through" middleware behaviors simple.

In today's API, command handlers are sometimes overridden at runtime. The --help and --version options do this, as do certain directives like [suggest] and [parse]. This is implemented in the middleware pipeline. The middleware checks the parse result for the presence of a given symbol and if it's found, it performs some action and then short circuits the command handler by not calling the pipeline continuation. This is flexible but hard to understand.

The CliAction design simplifies things by making these kinds of option actions peers to command actions, and then allowing interception to happen via pattern matching (a familiar language construct) rather than middleware (an often unfamiliar abstraction with a special delegate signature). A default order of precedence will be used if you simply call parseResult.Action.RunAsync(), but by optionally expanding the different CliAction types in a switch, you can change the order of precedence by just reordering the case statements. Or you can use if patterns. The code flow uses language concepts and is no longer hidden in the middleware.

There's one additional middleware use case that this doesn't cover in an obvious way, though, so I'll provide an illustration. This is the case when a middleware performs some action and then calls the continuation delegate. This enables behaviors that happen before and/or after the designated command's behavior.

Under this proposal, you would simply call the other action directly.

var parseResult = root.Parse("one");

switch (parseResult.Action)
{
    ParseErrorAction errorAction:
        // NOTE: The API to look up an invoke another action should be made simpler
        await new HelpAction(new HelpOption()).RunAsync();

        await parseResult.Action.RunAsync();

        break;
}

Additional implications

  • ICommandHandler will be replaced by CliAction, since the concepts are largely redundant and the name ICommandHandler won't make sense when applied to options or directives.

  • There will no longer be middleware APIs associated with CommandLineConfiguration.

@jonsequitur jonsequitur added needs discussion Further discussion required Area-API labels Mar 1, 2023
@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Mar 1, 2023

var from = parseResult.GetValue<FileInfo>();
var to = parseResult.GetValue<DirectoryInfo>();

This lookup looks type-based rather than name-based.

@jonsequitur
Copy link
Contributor Author

jonsequitur commented Mar 1, 2023

Thanks! Updated above. I forgot to add the option names as parameters. The generic type parameters are also needed since in this context, the compiler has no way to know what the correct type is.

@paulbartrum
Copy link

Under this proposal, would it be possible (and/or make sense) to skip the use of CliActions entirely?

i.e. something like this:

var rootCommand = new RootCommand()
{
  new Option<FileInfo>("--input")
};

var result = rootCommand.Parse(args);

// This is essentially the CliAction body, just without the cancellationToken.
Console.WriteLine($"Processing file {parseResult.GetValue<FileInfo>("--input").Name}...");

It seems like this would require fewer concepts and simplify the control flow in very basic scenarios. Good for those just getting started with this API (like me!)

Also, consider changing the name of RootCommand to CommandLineParser. While RootCommand may be technically more accurate, it's not clear to me as a beginner that commands even need to be organised into a tree, so it's not the name I'd be looking for when I'm just looking for a way to parse my command line. CommandLineParser fits my expectations better, plus it implies that you need to call the "Parse" method on it.

@jonsequitur
Copy link
Contributor Author

jonsequitur commented Mar 1, 2023

Under this proposal, would it be possible (and/or make sense) to skip the use of CliActions entirely?

Yes, you can just use the ParseResult directly, exactly like your example. The current API can be used that way, and this proposal won't change that.

@tmds
Copy link
Member

tmds commented Mar 13, 2023

@jonsequitur I'm reading through this, and I'm having trouble to see why ICommandHandler implementing types can't deliver what CliAction derived types bring? What is the difference?

Under this proposal, the code would now look like this:

imo, the CommandLineBuilder/CommandLineConfiguration configuration APIs are user-friendlier than the switch (result.Action) { case HelpAction helpAction: ... }.

@jonsequitur
Copy link
Contributor Author

ICommandHandler and CliAction are very similar and yes, well-known public types for ICommandHandler could mostly accomplish the same. The signature is basically identical. Anything you can do with ICommandHandler, you can do with CliAction. So I'll outline the main differences and motivations.

(And I'll say up front that assigment using a simple lambda will still be possible. And you won't have to pattern match in most cases.)

The first two are straighforward:

  • Preferring a class to an interface. This is a general .NET framework design guideline, and I expect as we implement it we'll have some default implementation details that will go in there.
  • Changing the name. This is because directives and options will also be able to have handlers, so the word "command" will become misleading.

Ok, here's the part that probably needs a little more explanation:

  • CliAction makes the types more important than the method signature.

Here's what this proposal should provide that ICommandHandler (and the way it's used today) doesn't:

  • You can see the intended outcome of a parsed command line using the public types of the chosen handler (including for error or short-circuit cases like parse errors and help). The current middleware design doesn't let you do this because the concrete ICommandHandler type has always been treated as an implementation detail.
  • These types become part of the API contract, which the concrete types of an interface generally are not.
  • It makes tooling, including for source generators, easier to build, because the APIs on the CliAction-derived types will be read/write APIs, whereas the CommandLineBuilder, middleware, and associated patterns are kind of write-only.
  • It then lets you defer the execution of code specific to each outcome. This will save some allocations. For example, help API configuration code won't be called unless the user actually asks for help.

We also hope this will also make the API a little more discoverable and progressive as people need more features. Here are the stages we have in mind:

  1. In the simplest case, you can just parse and do something with the result, without needing to use a CliAction at all:

    var result = rootCommand.Parse(args);
    
    if (result.GetValue("-a"))
    {
      // ...
    }
  2. If you need to something before and/or after execution, regardless of the handler, you can invoke the CliAction without caring which one it was:

    var result = rootCommand.Parse(args);
    
    if (result.GetValue("--dry-run"))
    {
      // do dry run stuff
      return; 
    }
    
    await result.Action.InvokeAsync();
  3. If you want to customize specific behaviors, you can match the CliAction-derived types you want to modify. (This is the switch example in the issue description). This should allow people do what middleware can do today, but with fewer abstractions:

    var result = rootCommand.Parse(args);
    
    switch (result.Action)
    {
        case HelpAction helpAction:
            await helpAction.RunAsync();
    
            Console.WriteLine("For more help see https://github.com/dotnet/command-line-api");
            break;
    
        default:
            await result.Action.RunAsync();
            break; 
    }

@tmds
Copy link
Member

tmds commented Mar 14, 2023

If I understand correctly, the reason for CliAction over ICommandHandler are non-functional. That is: HelpAction : ICommandHandler could serve the same functional behavior, but for the intended use-cases a class is preferred, and Action is a better name than CommandHandler.

I find properties are more discoverable than pattern matching:

switch (result.Action)
{
    case HelpAction helpAction:
        await helpAction.RunAsync();
        Console.WriteLine("For more help see https://github.com/dotnet/command-line-api");
        break;

vs

Xyz.HelpHandler = result => {
    defaultHelpAction = new HelpAction(result);
    defaultHelpAction.Run();
    Console.WriteLine("For more help see https://github.com/dotnet/command-line-api");
};

@tmds
Copy link
Member

tmds commented Mar 14, 2023

Just checking: in this proposal, CliAction gets added, and ICommandHandler is removed, right?
They don't exist simultaneously?

Why does CliAction take a ParseResult argument instead of a InvocationContext argument?

@jonsequitur
Copy link
Contributor Author

I find properties are more discoverable than pattern matching:

This is true if it's a fixed set, and it's something we discussed a bit. Any thoughts about how interception (examples 2 and 3 in my comment) might work with properties for the different handlers?

Just checking: in this proposal, CliAction gets added, and ICommandHandler is removed, right?
They don't exist simultaneously?

Correct. They're redundant concepts.

Why does CliAction take a ParseResult argument instead of a InvocationContext argument?

InvocationContext is going away and ParseResult will be passed everywhere that InvocationContext is passed today.

@Greg-Freeman
Copy link

Greg-Freeman commented Apr 4, 2023

You'll have to forgive me, as I am new at using this library. So, if there is a way to manage this, just let me know. But one thing I am having an issue with is if Command is inherited from in order to make a custom command, and the handler is configured inside the CustomCommand class, the handler is very tightly coupled to the fact that host is used. In the example below, context.GetHost() has to be called in order to get an instance of IPackager. This now makes the InstallCommand itself dependent on knowing how the command line was configured at application startup. It shouldn't need any knowledge of this.

When a generic base "Command" is used and configured from the top level, this is not an issue because the application startup code knows whether or not Hosting was used, and therefore can set the handler up correctly.

    internal class InstallCommand : Command
    {     
       public InstallCommand(string name, string? description = null) : base(name, description)
        {          
            var packageOption = new Option<string>("--package");
            packageOption.AddAlias("-p");

            AddOption(packageOption);

            this.SetHandler(context =>
            {
                var package = context.ParseResult.GetValueForOption(packageOption) ?? throw new NullReferenceException("package");
                IPackager packager = context.GetHost().Services.GetService(typeof(IPackager)) as IPackager ?? throw new NullReferenceException("packager");
                InstallOptions options = new(package);
                packager.InstallPackage(options);
            });
        }
    }

So, referring to this example, my initial reaction on how to fix this would be to either push the IPackager up to the constructor or the Handler method to the constructor. I don't really like the latter because it puts the burden on the caller to define a handler which defeats the purpose of abstracting it away in a child command. So then the other option is to pass the IPackager in through the constructor and use it as follows:

    internal class InstallCommand : Command
    {
       public InstallCommand(IPackager packager, string name, string? description = null) : base(name, description)
        {           
            var packageOption = new Option<string>("--package");
            packageOption.AddAlias("-p");

            AddOption(packageOption);

            this.SetHandler(context =>
            {
                var package = context.ParseResult.GetValueForOption(packageOption) ?? throw new NullReferenceException("package");
                
                InstallOptions options = new(package);
                packager.InstallPackage(options);
            });
        }
    }

But, there seems to be no good way of resolving the services from the host in order to use them in the InstallCommand constructor. But I may just be missing something.

 static async Task<int> Main(string[] args)
        {
            var rootCommand = CreateRootCommand();

            var builder= new CommandLineBuilder(rootCommand).UseHost(hostBuilder => {
                hostBuilder.ConfigureServices(services =>
                {
                    services.AddScoped<IPackagingService,PackagingService>();
                    services.AddSingleton<IPackager, Packager>();
                });
            rootCommand.AddCommand(new InstallCommand(//how do i get IPackager here));
            ......
            .....
            var parser = builder.Build();
            ...
         }
      

I feel as if we should be able to have some way of accessing the Host when creating commands so we can resolve services when constructing commands themselves.

@jonsequitur
Copy link
Contributor Author

jonsequitur commented Apr 4, 2023

@Greg-Freeman, the general guidance we've given on using DI with System.CommandLine is to do it as lazily as possible. This applies to both configuration and resolution of dependencies and applies whether or not you're using a host. This is because the parser might be used with no invocation of any CliAction (i.e. handler).

  • If your CLI app has multiple subcommands/verbs, then only one is chosen for invocation.
  • When users request completions, none of your actions will be called.

Under the new CliAction design, one approach that can be used is to subclass CliConfiguration and add you service provider there. The CliAction code itself can then ask for the services it needs. This reduces unnecessary work on the dependency resolution side.

To reduce unnecessary work on the dependency configuration side, we're also investigating making the instantiation of CliAction instances lazier, but there's no concrete proposal for that yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-API needs discussion Further discussion required
Projects
None yet
Development

No branches or pull requests

5 participants