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 4 #1750

Open
jonsequitur opened this issue Jun 2, 2022 · 54 comments
Open

Announcing System.CommandLine 2.0 Beta 4 #1750

jonsequitur opened this issue Jun 2, 2022 · 54 comments

Comments

@jonsequitur
Copy link
Contributor

jonsequitur commented Jun 2, 2022

System.CommandLine 2.0 Beta 4

We've been working through feedback on the beta 2 and beta 3 releases of System.CommandLine and now we're ready with beta 4. Here's a quick overview of what's new.

Microsoft Docs

Probably the most important update isn't in the API itself but in the release of preview documentation for System.CommandLine. It's been live now since April and, like the library, is in preview. There's a good deal of API to cover so if there's something that's missing or unclear, please let us know.

SetHandler simplification

The most common feedback we've heard about the SetHandler API introduced in beta 2 is that it's confusing, mainly because you can pass any number of IValueDescriptor<T> instances (e.g. Argument<T>, Option<T>, or BinderBase<T>) to the params IValueDescriptor[] parameter on most of the overloads, and this number can differ from the count of the handler's generic parameters.

I'll use this simple parser setup in the following examples:

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

Here's an example of the old SetHandler signature:

// ⚠️ Now outdated
public static void SetHandler<T1, T2>(
    this Command command,
    Action<T1, T2> handle,
    params IValueDescriptor[] symbols);

The expectation was that in the common case, you would call this method like this:

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

But the fact that the final parameter to the old SetHandler methods was a params array meant that the following would compile and then fail at runtime:

// ⚠️ Now outdated
rootCommand.SetHandler((int someInt, string someString) => { /* Do something exciting! */ });

The logic behind this method signature was that any handler parameters without a matching IValueDescriptor would be satisfied instead by falling back to BindingContext.GetService. This has been a longstanding way that dependency injection was supported for both System.CommandLine types such as ParseResult as well as for external types. But it's not the common case, and it's confusing.

We've changed the SetHandler methods to require a single IValueDescriptor<T> for each generic parameter T, rather than the previous params array:

 public static void SetHandler<T1, T2>(
    this Command command,
    Action<T1, T2> handle,
    IValueDescriptor<T1> symbol1,
    IValueDescriptor<T2> symbol2);

This has a number of benefits. In addition to making it clearer that you also need to pass the Option<T> and Argument<T> instances corresponding to the generic parameters, it also works with C# type inference to make your code a little more concise. You no longer need to restate the parameter types, because they're inferred:

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

We've also added overloads accepting InvocationContext, so if you want more detailed access for DI or larger numbers of options and arguments, you can do this:

rootCommand.SetHandler(
    (context) => // InvocationContext, but it's inferred so you don't need to specify it.
    {
        var i = context.ParseResult.GetValueForOption(option);
        var s = context.ParseResult.GetValueForArgument(argument);
        var someDependency = context.GetService(typeof(ILogger)) as ILogger;
        /* Do something exciting! */ 
    });

Finally, we've reduced the number of SetHandler overloads. Where before there were overloads supporting up to sixteen generic parameters, they now only go up to eight.

Option and Argument (non-generic) are now abstract

Support has existed since the earliest previews of System.CommandLine for creating non-generic Option and Argument instances that could be late-bound to a handler parameter. For example, an Argument could be created that could later be bound to either a string or an int parameter. This relied heavily on the reflection code that powered CommandHandler.Create, which was moved out of System.CommandLine and into the System.CommandLine.NamingConventionBinder package in beta 2. Fully removing support for this late-binding behavior by making Option and Argument abstract has helped simplify some code, clarify the API, and provided additional performance benefits.

Response file format unification

Response file support is a common feature of a number of command line tools, allowing a token within the command line to be replaced by inlining the contents of a file. You can think of a response file as a sort of configuration file that uses the same grammar as the command line tool itself. For example, given a file called response_file.rsp, the following command line will interpolate the contents of that file into the command line as if you had run it directly:

> myapp @response_file.rsp

(The .rsp file extension is another common convention, though you can use any file extension you like).

Until this release, System.CommandLine supported two different formats for response files: line-delimited and space-delimited. Line-delimited response files are more common and thus were the default. If response_file.rsp contained the following:

# Make it verbose
--verbosity
very 

then it would be treated as though you had run this:

> myapp --verbosity very

(Note following a #, the rest of the text on that line is treated as a comment and the parser will ignore it.)

This also provided a mechanism for avoiding the sometimes confusing rules of escaping quotes, which differ from one shell to another. You could produce a single token with a line-delimited response file by putting multiple words on a single line.

If response_file.rsp contained the following:

--greeting
Good morning! 

then it would be treated as though you had run this:

> myapp --greeting "Good morning!"

System.CommandLine also supported another response file format, space-separated, where the content of the response file was expected to be all on a single line and was interpolated verbatim into the command line. This was not commonly used and we decided it would simplify the developer choice and make the behavior clearer for end users to unify the format.

The rules for the new format are as follows:

  • Files can have many lines and content is treated as though concatenated into a single line.
  • One or more tokens can appear on a single line.
  • A line containing Good morning! will be treated as two tokens, Good and morning!.
  • If multiple tokens are intended to be passed as a single token (e.g. "Good morning!"), you must enclose them in quotes.
  • Shell-specific escaping rules are not used for the contents of a response file.
  • Any text between a # symbol and the end of the line is treated as a comment and ignored.
  • Tokens prefixed with @ can reference additional response files.

Custom token replacement

While we were trying to figure out how to make response file handling simpler, we saw that there are so many variations on how to parse response files that we could never provide a single implementation that could address them all. This could easily become a deal-breaker for people who have older apps that they would like to reimplement using System.CommandLine. So while we were consolidating response file handling, we also decided to generalize and hopefully future-proof the solution by adding a new extensibility point. After all, the core logic of response files is simple: during tokenization, a token beginning with @ is (possibly) replaced by zero or more other tokens.

If the default response file handling doesn't work for you, you can now replace it using the CommandLineBuilder.UseTokenReplacer method. The following example configures the parser to replace @-prefixed tokens with other tokens.

var parser = new CommandLineBuilder(command)
                .UseTokenReplacer((string tokenToReplace, out IReadOnlyList<string> replacementTokens, out string errorMessage) =>
                {
                    if (tokenToReplace == "fruits") // note that the leading '@' will already have been removed 
                    {
                        replacementTokens = new [] { "apple", "banana", "cherry" };
                        errorMessage = null;
                        return true;
                    }
                    else
                    {
                        replacementTokens
                        errorMessage = null; // no parse error is created
                        return false; // tokenToReplace is not replaced 
                    }
                })
                .Build();

This token replacement happens prior to parsing, so just as with response files, the new tokens can be parsed as commands, options, or arguments.

If you return true from the token replacer, it will cause the @-prefixed token to be replaced. If you return false, it will be passed along to the parser unchanged, and any value you assign to the errorMessage parameter will be displayed to the user.

Given the above token replacement logic, an app will parse as follows:

> myapp @fruits # is parsed as: myapp apple banana cherry
> myapp @not-fruits # is parsed as: myapp @not-fruits 

It's worth noting that this behavior is recursive, just as with response files. If a replacement token begins with @, the token replacer will be called again to potentially replace that token.

Added support for NativeAOT compilation

Continuing the work done in beta 3 to make apps built using System.CommandLine trimmable, they can now also be compiled with Native AOT. For more details, you can read about Native AOT compilation in the .NET 7 Preview 3 announcement here: https://devblogs.microsoft.com/dotnet/announcing-dotnet-7-preview-3/#faster-lighter-apps-with-native-aot.

@maxild
Copy link

maxild commented Jun 3, 2022

If before beta 4, I used a lambda with "magic" IConsole parameter (for instance (IConsole console, ...) => { /* code here */ } will I now have to use the InvocationContext based lambda, and use the GetValueForXXXX API to get the parsed option args?

rootCommand.SetHandler(context =>
// Using this line I cannot get to the IConsole console...
// rootCommand.SetHandler((env, module) =>
//
// Before beta 4 I used this line
// rootCommand.SetHandler((Environment? env, Module module, IConsole console) =>
{
    var module = context.ParseResult.GetValueForOption(optionModule);
    var env = context.ParseResult.GetValueForOption(optionEnvironment);
    // blah blah
    context.Console.Out.Write(response);
}

@jonsequitur
Copy link
Contributor Author

Yes. We wanted to limit the number of overloads. But it should be straightforward to define your own extension methods if you prefer to have the parameters for both injected objects and bound command line values in the same handler signature.

@bording
Copy link

bording commented Jun 3, 2022

@jonsequitur So with beta 4, if I want access to the IConsole or a cancellation token, I have to use the InvocationContext overload, correct?

That seems to mean that there no way to use a custom model binder and have access to a cancellation token at the same time.

If that is true, then this seems like an odd backstep to me.

@jonsequitur
Copy link
Contributor Author

@bording The InvocationContext should give you access to everything you need. You can get to the IConsole instance using InvocationContext.Console and you can get a CancellationToken (triggering Ctrl+C cancellation handling) by calling InvocationContext.GetCancellationToken().

@bording
Copy link

bording commented Jun 4, 2022

@jonsequitur I think you're missing my point. If I'm using the InvocationContext overload, how do I pass in my custom model binder? I've been able to abstract away the creation of several things that require multiple parameters and just get an instance of it in my handler through custom model binders.

I'm currently using custom model binders, and injecting an IConsole and a cancellation token. In beta 4, it seems like an either/or scenario.

@bgever
Copy link

bgever commented Jun 4, 2022

Before I was able to specify types on the handler that would automatically injected, and would resolve to the services registered in a middleware on startup.

So a Command class would look something like this:

using System.CommandLine;
using System.CommandLine.Binding;
using System.CommandLine.Invocation;
using System.CommandLine.IO;
using MyApp.Cli.Commands.User;
using MyApp.Cli.Primitives;
using MyApp.Cli.Services;

namespace MyApp.Cli.Commands;

public class UserCommand : Command
{
    public UserCommand() : base("user")
    {
        Description = "Display user authentication status";

        AddCommand(new LoginCommand());
        AddCommand(new LogoutCommand());

        var refreshOption = new Option<bool>("--refresh", "Force access token refresh even if hasn't expired yet");
        Add(refreshOption);

        this.SetHandler<bool, TokenService, Auth0Client, InvocationContext>(UserHandler, refreshOption);
    }

    public async Task UserHandler(
        bool refresh, TokenService tokenService, Auth0Client auth0Client, InvocationContext context)
    {
        var console = context.Console;
        var cancellationToken = context.GetCancellationToken();
        // [...]

But after this change I'm not able to easily inject both options/arguments and resolved services.
As a workaround, I made a small primitive class to inject services:

using System.CommandLine.Binding;
using Microsoft.Extensions.DependencyInjection;

namespace MyApp.Cli.Primitives;

public class Injectable<T> : BinderBase<T>
{
    protected override T GetBoundValue(BindingContext bindingContext)
    {
        return (T)bindingContext.GetRequiredService(typeof(T));
    }
}

That allows me to write the code like this, what looks even cleaner;

  • no more need to provide the generic types for the SetHandler, and
  • the service dependencies are clearly being marked as being injected.
using System.CommandLine;
using System.CommandLine.Invocation;
using System.CommandLine.IO;
using MyApp.Cli.Commands.User;
using MyApp.Cli.Primitives;
using MyApp.Cli.Services;

namespace MyApp.Cli.Commands;

public class UserCommand : Command
{
    public UserCommand() : base("user")
    {
        Description = "Display user authentication status";

        AddCommand(new LoginCommand());
        AddCommand(new LogoutCommand());

        var refreshOption = new Option<bool>("--refresh", "Force access token refresh even if hasn't expired yet");
        Add(refreshOption);

        this.SetHandler(
            UserHandler,
            refreshOption,
            new Injectable<TokenService>(),
            new Injectable<Auth0Client>(),
            new Injectable<InvocationContext>());
    }

    public async Task UserHandler(
        bool refresh, TokenService tokenService, Auth0Client auth0Client, InvocationContext context)
    {
        var console = context.Console;
        var cancellationToken = context.GetCancellationToken();
        // [...]

Maybe this type of Injectable helper can be added to the library?

@WeihanLi
Copy link

WeihanLi commented Jun 4, 2022

Should I use the Option<bool> instead of Option since the the Option is abstract now

@jonsequitur
Copy link
Contributor Author

Should I use the Option<bool> instead of Option since the the Option is abstract now

Yes. When you declare options and arguments, you'll need to use Option<T> and Argument<T>.

You can use Option and Argument (or their base type, Symbol) when iterating over collections that might contain different generic types of Option<T> and Argument<T>, e.g.:

foreach (Option opt in command.Options)
{
}

@tomrus88
Copy link

tomrus88 commented Jun 6, 2022

Finally, we've reduced the number of SetHandler overloads. Where before there were overloads supporting up to sixteen generic parameters, they now only go up to eight.

So what should I do now if I used one of removed overloads? I was passing 11 of Option<T> into SetHandler.

@wuzzeb
Copy link

wuzzeb commented Jun 6, 2022

Now that options are required to be passed to SetHandler (I like this change), how about allowing us to omit also adding them directly to the command. In other words, any option passed to SetHandler is implicitly also added to the root command.

@jonsequitur
Copy link
Contributor Author

Now that options are required to be passed to SetHandler (I like this change), how about allowing us to omit also adding them directly to the command.

We've looked into this and while it works for the simple case, there are a number of cases where this API would lead to ambiguities. I outlined a few here: #1537 (comment)

@jonsequitur
Copy link
Contributor Author

So what should I do now if I used one of removed overloads? I was passing 11 of Option into SetHandler.

@tomrus88, you can either use a custom BinderBase<T> or the SetHandler overloads with a delegate that accepts an InvocationContext. Both approaches are described here: https://docs.microsoft.com/en-us/dotnet/standard/commandline/model-binding#parameter-binding-more-than-8-options-and-arguments

@NickStrupat
Copy link

NickStrupat commented Jun 7, 2022

Hi folks,

I needed the CancellationToken in my handlers but I wanted to use the SetHandler helper method. I came up with this class which provides the CancellationToken as an IValueDescriptor<T>.

var name = new Option<String>("name", "Your name!");
var ctvs = new CancellationTokenValueSource();
var root = new RootCommand("Root command!") { name };

root.SetHandler(async (name, ct) => {
	Console.Write("name: ");
	try { await Task.Delay(5000, ct); }
	finally { Console.WriteLine(name); }
}, name, ctvs);

var clb = new CommandLineBuilder(root);
clb.CancelOnProcessTermination();
return await clb.Build().InvokeAsync(args);
class CancellationTokenValueSource : IValueDescriptor<CancellationToken>, IValueSource
{
	public Boolean TryGetValue(IValueDescriptor valueDescriptor, BindingContext bindingContext, out Object? boundValue)
	{
		boundValue = (CancellationToken)bindingContext.GetService(typeof(CancellationToken))!;
		return true;
	}

	public Object? GetDefaultValue() => throw new NotImplementedException();
	public String ValueName => throw new NotImplementedException();
	public Type ValueType => throw new NotImplementedException();
	public Boolean HasDefaultValue => throw new NotImplementedException();
}

Run dotnet run -- name nick and use CTRL+C before the 5 seconds wait is up to see the behaviour.

@FroggieFrog
Copy link

After updating from beta3 to beta4 I have several erros with the following snippet.

testCommand.SetHandler<int, bool, FileInfo, bool, IConsole>((intOption, boolOption, fileOption, verbose, console) =>
{
    if (verbose)
    {
        console.Out.Write(nameof(addTestCommand));
        console.Out.Write(Environment.NewLine);
    }

    _logger.Info($"Command used: {testCommand.Name}{Environment.NewLine}" +
        $"\t{nameof(intOption)}: \"{intOption}\"{Environment.NewLine}" +
        $"\t{nameof(boolOption)}: \"{boolOption}\"{Environment.NewLine}" +
        $"\t{nameof(fileOption)}: {fileOption}");

    console.Out.Write($"The value for --int-option is: {intOption}");
    console.Out.Write(Environment.NewLine);
    console.Out.Write($"The value for --bool-option is: {boolOption}");
    console.Out.Write(Environment.NewLine);
    console.Out.Write($"The value for --file-option is: {fileOption?.FullName ?? "null"}");
    console.Out.Write(Environment.NewLine);

    if (verbose)
    {
        console.Out.Write($"Done!");
    }
}, optionInt, optionBool, optionFile, optionVerbose);
  1. error CS1643 "Not all code paths return a value in lambda expression of type Func<int, bool, FileInfo, bool, IConsole, Task>"
    I can fix this by adding await Task.CompletedTask; before the end and making the lambda async.

  2. after the first error is fixed there appears error CS1501 "No overload for method SetHandler takes 5 arguments"
    I tried to fix this by replacing IConsole with InvocationContext, but with no success.

So this is my updated still not working code:

testCommand.SetHandler<int, bool, FileInfo, bool, InvocationContext>(async (intOption, boolOption, fileOption, verbose, context) =>
{
    if (verbose)
    {
        context.Console.Out.Write(nameof(addTestCommand));
        context.Console.Out.Write(Environment.NewLine);
    }

    _logger.Info($"Command used: {testCommand.Name}{Environment.NewLine}" +
        $"\t{nameof(intOption)}: \"{intOption}\"{Environment.NewLine}" +
        $"\t{nameof(boolOption)}: \"{boolOption}\"{Environment.NewLine}" +
        $"\t{nameof(fileOption)}: {fileOption}");

    context.Console.Out.Write($"The value for --int-option is: {intOption}");
    context.Console.Out.Write(Environment.NewLine);
    context.Console.Out.Write($"The value for --bool-option is: {boolOption}");
    context.Console.Out.Write(Environment.NewLine);
    context.Console.Out.Write($"The value for --file-option is: {fileOption?.FullName ?? "null"}");
    context.Console.Out.Write(Environment.NewLine);

    if (verbose)
    {
        context.Console.Out.Write($"Done!");
    }

    await Task.CompletedTask;
}, optionInt, optionBool, optionFile, optionVerbose);

Is this no longer supported or do I use the library wrong?

@NickStrupat
Copy link

@bgever I just realized my solution is almost the same as yours, but mine's worse haha. Thanks for posting it!

@jonsequitur
Copy link
Contributor Author

@bgever @NickStrupat I hit on more or less the same solution.

@FroggieFrog You could use this same pattern to bind your IConsole parameter. The rest of your original code should then work as-is.

@FroggieFrog
Copy link

So what "just worked" before, now needs to be done by hand with way more lines of code?
I really hope this will work once again in a future update.

@lejsekt
Copy link

lejsekt commented Jun 9, 2022

Hi,
is there a way to define whether an option is nullable or not? I was using var uriOption = new Option<Uri>(...) in beta 3 and would use it as command.SetHandler(async (Uri uri) => { ... }, uriOption);.
But now , if I use var uri = context.ParseResult.GetValueForOption(uriOption); (as I need the InvocationContext to change exit codes, use cancellation token etc.), the inferred type is Uri? and nullable analysis requires to handle that case. Is there a better way to handle it? Thank you

@amirmokarram
Copy link

Hi,
I encountered a problem with Option and Argument that abstract now, because in my solution Commands is dynamically discovered and after that create Option and Argument if needed. But now, create Option and Argument like this Type dynamicOptionType = typeof(Option<>).MakeGenericType(x); and after that instantiate like this Option option = (Option)Activator.CreateInstance(dynamicOptionType). Therefore, from my point of view this change is not suitable.

@jonsequitur
Copy link
Contributor Author

So what "just worked" before, now needs to be done by hand with way more lines of code?

@FroggieFrog Here's the implementation I'm using. It might be reasonable to add this into System.CommandLine, but in the meantime it's not much code:

internal static class Bind
{
    public static ServiceProviderBinder<T> FromServiceProvider<T>() => ServiceProviderBinder<T>.Instance;

    internal class ServiceProviderBinder<T> : BinderBase<T>
    {
        public static ServiceProviderBinder<T> Instance { get; } = new();

        protected override T GetBoundValue(BindingContext bindingContext) => (T)bindingContext.GetService(typeof(T));
    }
}

With this, your sample would pass one extra to SetHander using this method:

testCommand.SetHandler((intOption, boolOption, fileOption, verbose, console) => // 👈 parameter types are now inferred
{
    // ...
}, optionInt, optionBool, optionFile, optionVerbose, Bind.FromServiceProvider<IConsole>()); // 👈 call service provider binder here

@FroggieFrog
Copy link

@jonsequitur
Thank you for your time and the snippet. It works as expected.

It might be reasonable to add this into System.CommandLine

I really hope this will be added.

@bgever
Copy link

bgever commented Jun 15, 2022

Like your solution @jonsequitur!

FYI, I'm using the extension method GetRequiredService<T>(this IServiceProvider provider) from Microsoft.Extensions.DependencyInjection to play nicely with <Nullable>enable</Nullable> in csproj.

@Balkoth
Copy link

Balkoth commented Jun 15, 2022

I am posting my sample from last time, which this update of SetHandler broke again:

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;
  }
}

You can not be serious that i have to specify the type and position each time i want to add the option. With every release you offload more and more to the user of the library making it more complex to use.

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,  (IValueDescriptor<T1>)options[0], (IValueDescriptor<T2>)options[1]);
    return command;
  }
}

@Balkoth
Copy link

Balkoth commented Jun 15, 2022

I scoured through the sample on how to pass arguments to SetHandler is supposed to be used:

var delayOption = new Option<int>
    ("--delay", "An option whose argument is parsed as an int.");
var messageOption = new Option<string>
    ("--message", "An option whose argument is parsed as a string.");

var rootCommand = new RootCommand("Parameter binding example");
rootCommand.Add(delayOption);
rootCommand.Add(messageOption);

rootCommand.SetHandler(
    (delayOptionValue, messageOptionValue) =>
    {
        DisplayIntAndString(delayOptionValue, messageOptionValue);
    },
    delayOption, messageOption);

await rootCommand.InvokeAsync(args);

This code looks like it is written by someone who is writing code for the first time. There is so much duplication in it that i am wondering who is approving this stuff.

@jonsequitur
Copy link
Contributor Author

This code looks like it is written by someone who is writing code for the first time. There is so much duplication in it that i am wondering who is approving this stuff.

@Balkoth We'd love to hear any productive suggestions, but the tone of this comment isn't that.

Ok, so the decision to not couple the parsing API to the parameter binding API is deliberate. Creating wrapper libraries that make this more concise is trivial but it requires embedding conventions that limit the CLI grammars that can be supported by the parser, so they probably belong at a higher layer. But for a given codebase, if you don't need some of those features, you can be more opinionated.

Here's a refactor of your sample:

internal static class Program
{
    private static int Main(string[] args)
    {
        // Create a root command containing the sub commands.
        RootCommand rootCommand = new RootCommand
        {
            new Command("export", "Writes files containing specified export options.")
                .WithHandler(HandleExport,
                             new Option<bool>("--reports", "Exports reports."),
                             new Option<bool>("--files", "Exports files.")),
            new Command("check", "Checks the specified options.")
                .WithHandler(HandleCheck,
                             new Option<bool>("--reports", "Checks reports."),
                             new Option<bool>("--files", "Checks files."))
        };

        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, IValueDescriptor<T1> symbol1, IValueDescriptor<T2> symbol2)
    {
        command.Add(symbol1);
        command.Add(symbol2);
        command.SetHandler(handler, symbol1, symbol2);
        return command;
    }
}

This change to WithHandler breaks the array into separate parameters which a) enables type inference so we don't have to specify the generic type parameters and b) avoids the (admittedly minor) performance overhead of an iterator. It's also more concise.

In fact, we've explored making SetHandler look more like this.

There are problems. It's unclear how global options would work.

Here's another. Consider this parser setup:

var option = new Option<int>("-i");
var subcommand1 = new Command("subcommand1");
subcommand1.SetHandler(i => {}, option);
var subcommand2 = new Command("subcommand2");
subcommand2.SetHandler(i => {}, option);
var root = new RootCommand
{
    option,
    subcommand1,
    subcommand2
};

The option -i is declared on the root command, making this valid:

> myapp -i subcommand1

But as you can see from the SetHandler calls, option can be bound to the nested subcommands' handlers.

Put differently, the parser API defines the grammar, while the SetHandler API defines how to bind symbols regardless of their position in the parsed input.

Suggestions are welcome for making this more concise without introducing coupling. Our current plan though is to keep them decoupled, improve the ergonomics to the degree possible given that constraint, and allow additional layers (in some cases source generator-driven) to provide more terse and opinionated APIs.

@Balkoth
Copy link

Balkoth commented Jun 16, 2022

Imho you keep making weird decisions by offloading more and more development work to me with each release. I used this library so that i don't have to create wrapper libraries. Your changes and justifications for it alienate me so hard, that i may just look for another solution.

This is what your changes have done so far. I was excited at first l, that finally a command line parsing library from microsoft is around. But then it turns into such a chore.

P.S.: Don't shoot the messger.

@jonsequitur
Copy link
Contributor Author

Those of us working on System.CommandLine are also not satisfied yet with this API, which is why there's still a beta tag on it.

The changes so far have been to remove ambiguity from the API because both CommandHandler.Create and Command.SetHandler<...>(..., params IValueDescriptor[] symbols) relied on conventions that far too many people were confused by. These conventions were attempts to reduce verbosity.

On the subject of wrapper libraries, people will write them because they want their own conventions, but we fully intend to make System.CommandLine work well on its own.

@lonix1
Copy link

lonix1 commented Sep 1, 2022

For the code posted by @jonsequitur (Bind) or similarly by @bgever (Injectable), does someone know where those services are actually registered?

Using those samples without that step fails:

Unhandled exception: System.InvalidOperationException: No service for type 'MyService' has been registered.

@JobaDiniz
Copy link

@lonix1 you should register them in the Middleware:

var parser = new CommandLineBuilder(new RootCommand())
                .AddMiddleware(Middleware)
                .Build();

private static async Task Middleware(InvocationContext context, Func<InvocationContext, Task> next)
        {
            context.BindingContext.AddService(s => YOUR SERVICE INSTANCE);

            await next(context);
        }

@lonix1
Copy link

lonix1 commented Sep 1, 2022

@JobaDiniz Of all the places I looked, registering services in middleware didn't even cross my mind. This library's API is really odd. 😄 Thanks for your help!

(I actually did register services in UseHost via the generic host plugin, but that was ignored.)

@jonsequitur
Copy link
Contributor Author

@JobaDiniz:

Question 1: did this release remove the CommandHandler.Create<...> class completely? I've seen examples using it, but I'm using 2.0.0-beta4.22272.1 and such class does not exist anymore.

It was moved into the System.CommandLine.NamingConventionBinder package because it relies heavily on reflection and conventions. You can still use it but trimmable or NativeAOT-compiled apps are not possible with that API.

Question 2: is there a reason why this library does not provide support for creating "handler" classes with the command definition parameters? Maybe there was a discussion and some reasoning, but I just wish I could define separated classes to handle the commands, and not have to use lambda expressions.

We're working on simplified API approaches to layer on top of the core parser, which is intentionally more low-level. This will be shipped in a separate package.

@lonix1

Of all the places I looked, registering services in middleware didn't even cross my mind.

This is definitely unintuitive and another place where an additional API layer will help. The reason it's there is that CLI apps are often very short-lived so we want to do any registration work lazily. For example, during command line completion, System.CommandLine performs parsing but not invocation, so registered services are not used.

@lonix1
Copy link

lonix1 commented Sep 1, 2022

@JobaDiniz:

CLI apps are often very short-lived so we want to do any registration work lazily

Been considering that... I agree IoC may be overkill since we're invoking just one command. In that case, why use it at all? Why not just define/instantiate services within each command/handler. There are probably use cases I've not encountered, but right now I'm gravitating toward avoiding IoC completely; for simple cli apps I see no benefit other than more abstraction and busywork.

define separated classes to handle the commands, and not have to use lambda expressions.

I also want to define commands and/or handlers in separate classes, and found a way to do so. But when the API is redesigned, it would be nice to be able to use lambdas anyway, if we want them. The alternative is do it the way all the other libraries do, which is to use attributes for everything. That makes the code cleaner, but then everything must be compile-time constant. One cannot define names, descriptions, etc., as anything other than const, it's harder to localise, etc.

@jonsequitur
Copy link
Contributor Author

In that case, why use it at all? Why not just define/instantiate services within each command/handler.

Many people asked for this so we simply exposed the internal mechanism that was already in place to support injection of System.CommandLine types. To your point, in the straw man example above, Bind.FromServiceProvider is proposed as a way to wire up DI on a per-command basis and the implementation would be lazy.

I also want to define commands and/or handlers in separate classes, and found a way to do so. But when the API is redesigned, it would be nice to be able to use lambdas anyway, if we want them. The alternative is do it the way all the other libraries do, which is to use attributes for everything. That makes the code cleaner, but then everything must be compile-time constant. One cannot define names, descriptions, etc., as anything other than const, it's harder to localise, etc.

I expect all of these uses cases to remain intact through any API reworking to come. All three are valid preferences. Currently, the lambda is a shortcut to defining an ICommandHandler and matches the signature of ICommandHandler.InvokeAsync(InvocationContext). The SetHandler overloads are intended as convenience methods over that method. The same will be true of any source generator-driven approaches (#1829), potentially including attribute-decorated APIs. Attribute-decorated APIs have also been requested a number of times in the past but we've avoided them so as not to build reflection requirements into the parser.

@lonix1
Copy link

lonix1 commented Sep 2, 2022

Many people asked for this

Yes I quickly realised that's the problem here. There are so many stakeholders (many inside MS) and everyone wants something else, according to different design expecations. I don't envy you your job in this, must be tough to keep everyone happy. 😄

I think the lowest common denominator is to design an API that follows the Principle of Least Surprise, and implement weird requirements on top of that.

@JamesNK
Copy link
Member

JamesNK commented Oct 17, 2022

IMO I think you should reconsider whether the pattern of SetHandler with generic arguments is a good idea. Every 12 months I have update an app to the latest version of System.Commandline, and each time there is a lot of upgrade pain. I was using SetHandler with generic arguments, which have been made progressively more and more restrictive.

It's clear that the handler was originally designed to be dynamic and easy to use, but that conflicts with new requirements to make the library strongly typed and compatible with AOT. Unfortunately SetHandler has had to change so much that it feels like as much work to use as pulling values out of the InvocationContext inside the handler, except more restrictive.

Today I had to entirely drop using generic arguments everywhere because I need InvocationContext. These two features are no longer compatible. A good design should allow for something to become progressively more complex. Recommending using generic arguments with SetHandler for simple scenarios, then requiring handlers to be rewritten to get values manually with a context isn't a good experience.

Maybe SetHandlerWithContext is a solution. Or always make InvocationContext the first argument, even with generic arguments. Or just not have generic argument overloads and always use the overload that just takes the context.

@gao-artur
Copy link

Hi folks,

I needed the CancellationToken in my handlers but I wanted to use the SetHandler helper method. I came up with this class which provides the CancellationToken as an IValueDescriptor<T>.

var name = new Option<String>("name", "Your name!");
var ctvs = new CancellationTokenValueSource();
var root = new RootCommand("Root command!") { name };

root.SetHandler(async (name, ct) => {
	Console.Write("name: ");
	try { await Task.Delay(5000, ct); }
	finally { Console.WriteLine(name); }
}, name, ctvs);

var clb = new CommandLineBuilder(root);
clb.CancelOnProcessTermination();
return await clb.Build().InvokeAsync(args);
class CancellationTokenValueSource : IValueDescriptor<CancellationToken>, IValueSource
{
	public Boolean TryGetValue(IValueDescriptor valueDescriptor, BindingContext bindingContext, out Object? boundValue)
	{
		boundValue = (CancellationToken)bindingContext.GetService(typeof(CancellationToken))!;
		return true;
	}

	public Object? GetDefaultValue() => throw new NotImplementedException();
	public String ValueName => throw new NotImplementedException();
	public Type ValueType => throw new NotImplementedException();
	public Boolean HasDefaultValue => throw new NotImplementedException();
}

Run dotnet run -- name nick and use CTRL+C before the 5 seconds wait is up to see the behaviour.

Thanks @NickStrupat, that worked! But make sure to call .UseDefaults() on var clb = new CommandLineBuilder(root); otherwise you will lose help, argument validation, error code propagation, and who knows what else. The .UseDefaults() calls CancelOnProcessTermination internally so you can omit this from your code.

@davidcok
Copy link

July 11: Would be nice to have an option on the validation parts to configure whether help is displayed or not (if others want the opposite behavior).

For cases in which the help document describing all the options is quite long, there is a definite need to be able to control when the full help document is shown and when just an error message about the command-line is shown. Cf. #1214

@micdah
Copy link

micdah commented Mar 14, 2023

sixteen

I encountered this issue also, but found a way around it.

By looking into the source code a bit for how values for options / arguments are retrieved, I wrote this extension class

public static class CliExtensions
{
    public static T GetValue<T>(this InvocationContext ctx, IValueDescriptor<T> symbol)
    {
        if (symbol is IValueSource valueSource &&
            valueSource.TryGetValue(symbol, ctx.BindingContext, out var boundValue) &&
            boundValue is T value)
        {
            return value;
        }

        return symbol switch
        {
            Argument<T> argument => ctx.ParseResult.GetValueForArgument(argument),
            Option<T> option => ctx.ParseResult.GetValueForOption(option),
            _ => throw new ArgumentOutOfRangeException()
        };
    }
}

With it, I can just

var option1 = new Option<bool>("--option1");
var option2 = new Option<bool>("--option2");

var command = new Command("test"){ option1, option2 };
command.SetHandler((ctx) => {
  var value1 = ctx.GetValue(value1);
  var value2 = ctx.GetValue(value2);
  
  // Do stuff
});

I suppose I could even just add my own extension methods to Command with additional SetHandler methods taking more than 8 options / arguments - but this also works.

Rather odd to have an arbitrary upper-bound on number of options / arguments, especially set at 8. Would have made sense to at least have a separate overload which allowed any number of options but less readable / friendly as with typed overloads - just to ensure cases where more than 8 is needed, are still possible, if not nearly as "neat" looking.

@KalleOlaviNiemitalo
Copy link

@micdah, the SetHandler overloads were removed yesterday in #2089. There are now only Command.SetHandler(Action<InvocationContext>) and Command.SetHandler(Func<InvocationContext, CancellationToken, Task>), and the handlers have to query the values from InvocationContext, most conveniently using the InvocationContext.GetValue methods that were added in #1787.

InvocationContext itself is also going to be replaced with ParseResult, according to #2046 (comment).

@maxandriani
Copy link

Same to me.

The BindingContext provides the Add Service that we use to register dependencies. But the lack of an IServiceCollection configuration step really difficult my job to register libraries that provides clean and simple extensions methods.

@scanyard
Copy link

@micdah, the SetHandler overloads were removed yesterday in #2089. There are now only Command.SetHandler(Action) and Command.SetHandler(Func<InvocationContext, CancellationToken, Task>), and the handlers have to query the values from InvocationContext, most conveniently using the InvocationContext.GetValue methods that were added in #1787.

InvocationContext itself is also going to be replaced with ParseResult, according to #2046 (comment).

Is there a plan to update the public docs on learn about this change? As of now, multiple pages on there, including the tutorial landing page are broken. I appreciate this is Beta but it's not a good onboarding experience.

@tksrc
Copy link

tksrc commented Sep 17, 2023

Sorry if asked before but when can we expect a GA release?

@vladboss61
Copy link

vladboss61 commented Dec 14, 2023

Sorry if asked before but when can we expect a GA release?

I think never we have the release of its. :D
We are going to leave in beta version until the end of the world.

@tksrc
Copy link

tksrc commented Dec 14, 2023

Sorry if asked before but when can we expect a GA release?

I think never we have the release of its. :D We are going to leave in beta version until the end of the world.

Not ideal :)

zwoefler added a commit to zwoefler/VmChamp that referenced this issue Jun 30, 2024
When trying to add the user option ran into error:
"No overload for method 'SetHandler' takes 10 arguments"

The Docs for SetHandler are wrong!!

LINKS:
GITHUB ISSUE: dotnet/command-line-api#1750
DOCS: https://learn.microsoft.com/en-us/dotnet/standard/commandline/model-binding#parameter-binding-more-than-8-options-and-arguments
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