Add SetHandler overrides for command returning an exit code

[dgolub]

The original CommandHandler.Create API supports handler functions that return int so that the command can return an exit code other than 0 if there's an error. It looks like the new SetHandler API requires that the handler function return either void or Task. Can we add overrides for int and Task<int> before GA? Thanks!

[jonsequitur]

You can always take in an InvocationContext in your handler and set InvocationContext.ExitCode.

[baronfel]

We had this same situation on the dotnet CLI, and the approach we took was to:

• make our own ICommandHandler that fit the lambda signature we needed and transformed our lambdas into Task<int> returns
• made an SetHandler extension method on the Command type (like is already done in this repo) for our specific use case to create our ICommandHandler instead

so not a ton of work, but a slight change in the expected API.

[jonsequitur]

It's hard to know what's the right number of overloads. I think there are too many and it makes intellisense a bit confusing. (The source generator approach we've been experimenting with is basically just generating the exact signature you need.)

Doing away with return types (other than simple Task to support async) can help keep this under control and leaves the door open to have other return types from handlers, e.g. data objects that a rendering layer can then present in the console.

[tmds]

Commands are expected to handle failure situations and turn them into exit codes. I'm surprised SetHandler API doesn't foresee this.

The current API encourages throwing exceptions over returning exit codes. That gives a bad UX for the end-user.

[tmds]

You can always take in an InvocationContext in your handler and set InvocationContext.ExitCode.

@jonsequitur does that mean I can't use any of the overloads that take a strongly-typed Action<T1, ..., Tx>?

[jonsequitur]

In beta 4, in order to keep the combinatorial overload explosion to a minimum, we only provide two flavors of SetHandler:

• Those that bind parameters for you, like this:

  var option = new Option<int>("-i");
  // ...
  command.SetHandler(i => { }, option);
  

  (These parameters can include types like CancellationToken or InvocationContext using a custom binder, which we'll simplify in beta 5 by introducing something like Bind.FromServiceProvider<T>().)

• Those that provide an InvocationContext, like this:

  var option = new Option<int>("-i");
  // ...
  command.SetHandler(context => 
  { 
      var i = context.ParseResult.GetValueForOption(option);
  });
  

The current API encourages throwing exceptions over returning exit codes.

Exceptions are caught and translated into non-zero exit codes by default, or you can assign the exit code to InvocationContext.ExitCode.

Ok, so where does that leave us? Right now there are 20 SetHandler overloads, accepting these delegate types:

• Action
• Action<InvocationContext>
• Action<T1> through Action<T1, T2, T3, T4, T5, T6, T7, T8>
• Func<Task>
• Func<InvocationContext, Task>
• Func<T1, Task> through Action<T1, T2, T3, T4, T5, T6, T7, T8, Task>

If you return Task<int> from any of the Func* overloads, it will be used to set the exit code:

command.SetHandler(i =>
{
    return Task.FromResult(123);
}, option);

The problem here is that you get a compiler error if you try to use an async delegate:

// THIS DOES NOT COMPILE IN BETA 4
command.SetHandler(async i =>
{
    return 123;
}, option);

Supporting directly returning an exit code would double the number of SetHandler overloads, adding:

• Func<int>
• Func<InvocationContext, int>
• Func<T1, int> through Action<T1, T2, T3, T4, T5, T6, T7, T8, int>
• Func<Task<int>>
• Func<InvocationContext, Task<int>>
• Func<T1, Task<int>> through Action<T1, T2, T3, T4, T5, T6, T7, T8, Task<int>>

This would be a usability improvement in some ways and not in others. IntelliSense is already quite crowded for this method.

Adding support for directly returning exit codes would nearly double this. We're looking for a more elegant solution and would love suggestions. This is related to #1776 so I'll add these details there.

[dgolub]

Is there a particular exception class that can be used to specify an exit code and error message, or do we need to define our own exceptions?

[jonsequitur]

You could define you own if you wanted to take this approach. I don't encourage using exceptions to communicate with the user.

[dgolub]

Right, which is why a number of us have been asking for a simple way to return an error code.

[jonsequitur]

I suppose another option that wouldn't result in an overload explosion is to require returning an exit code on all of the SetHandler APIs. It would be a fairly disruptive change and it's not clear to me which is the more common pattern.

[dgolub]

I would suspect that there are developers who have strong preferences for both, much like with int main() and void main() in C/C++.

[tmds]

(These parameters can include types like CancellationToken or InvocationContext using a custom binder, which we'll simplify in beta 5 by introducing something like Bind.FromServiceProvider<T>().)

Ack. This is what I need to combine Options and InvocationContext with the strongly-typed Action. I'll wait for beta5.

I suppose another option that wouldn't result in an overload explosion is to require returning an exit code on all of the SetHandler APIs. It would be a fairly disruptive change and it's not clear to me which is the more common pattern.

I think that encourages the use of exit codes and printing user-friendly error messages over throwing Exception, which will be good for the end-user.

Maybe also consider adding something like:

public static class ExitCode
{
    public const int Success = 0;
    public const int Failure = 1;
}

So users can write return ExitCode.Success; over return 0;.

[tmds]

Maybe we should use enums to represent command results?

The SetHandler methods can have an enum constraint:

    void SetHandler<T>(Func<Task<T>> handle) where T : System.Enum;
    void SetHandler<T>(Func<T> handle) where T : System.Enum;
    ...

We can include a default enum:

enum CommandResult
{
    Success = 0,
    Failure = 1
}

Example with a custom enum:

enum MyCommandStatus
{
     Success = 0,
     Failure1 = 100,
     Failure2 = 200
}
command.SetHandler(() => MyCommandStatus.Failure1);

[tmds]

@KalleOlaviNiemitalo since you gave it a thumbs down, can you elaborate on what you don't like about the enum suggestion?

[KalleOlaviNiemitalo]

There's no way to constrain it to enums in which the underlying type is int.

[KalleOlaviNiemitalo]

On Windows, there is some precedent for using a Win32 error code, HRESULT, or NTSTATUS as an exit code of a process, but those do not have complete enum types and it would be weird if one had to define an enum type just to satisfy SetHandler.

The proposed API would also allow command.SetHandler<Enum>(() => null), but that's easy to fix.

[tmds]

There's no way to constrain it to enums in which the underlying type is int.

Right, we can't constrain the enum to have underlying int type. This is more an implementation concern.

it would be weird if one had to define an enum type just to satisfy SetHandler.

The alternatives are to use const fields or literal constants.

[KalleOlaviNiemitalo]

I mean, if you want to return Exception.HResult, it would look like this

command.SetHandler(() =>
    {
        try
        {
            …
            return (HResult)0;
        }
        catch (Exception exception)
        {
            return (HResult)exception.HResult;
        }
    });

// Type not used elsewhere, but SetHandler requires an enum for the exit code
enum HResult {}

[KalleOlaviNiemitalo]

The UseExceptionHandler, UseParseDirective, and UseParseErrorReporting methods of CommandLineBuilderExtensions take the exit code as an int? parameter. It would be unfortunate if SetHandler were inconsistent with these and supported enum types but not int.

https://github.com/dotnet/command-line-api/blob/350a618ab44932c568ae2392d7a571281baed59a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs#L319-L322 https://github.com/dotnet/command-line-api/blob/350a618ab44932c568ae2392d7a571281baed59a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs#L495-L497 https://github.com/dotnet/command-line-api/blob/350a618ab44932c568ae2392d7a571281baed59a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs#L520-L522

[tmds]

That some of these work with int doesn't mean SetHandler needs to do the same. Similar to how ASP.NET doesn't make you work with HttpContext when implementing an endpoint.

Anyway, I'll be happy to have a directy way to control the exit code, be it an int or an enum.

The maintainers can decide what they want.

[tmds]

@jonsequitur if you decide what the API should be, I can look into implementing it, if you want.

[tmds]

This is fixed by https://github.com/dotnet/command-line-api/pull/2092.

[KalleOlaviNiemitalo]

This is no longer fixed. In https://github.com/dotnet/command-line-api/pull/2095, the SetHandler(Func<InvocationContext, int>) and SetHandler(Func<InvocationContext, CancellationToken, Task<int>>) methods were replaced with SetAction(Action<InvocationContext>) and SetAction(Func<InvocationContext, CancellationToken, Task>), so it is no longer easy to return an exit code from a handler delegate, especially now that the InvocationContext.ExitCode property has been deleted. It can still be done but the solutions have drawbacks:

• By deriving an application-specific class from CliAction and setting the Command.Action property. That requires somewhat more code.
• By making System.CommandLine.SourceGenerator generate the application-specific action class. This is only available for C# and does not work in older versions of Visual Studio that are still supported.
• Perhaps by using CommandHandler.Create from the System.CommandLine.NamingConventionBinder package. This would be an additional dependency that needs to be shipped with the application.

See also https://github.com/dotnet/command-line-api/issues/2109 for the async case.