Return values of CliActions should be more than an integer

[KathleenDollard]

Within execution we have richer information than an int. We need to share this information.

Rust and Python’s argParse both use structures here and I think this is essential. I think we can maintain simplicity for the simple case – for example we could have a type that could be implicitly converted to int.

The key data for the return structure is:

• The integer return code
• A message to display to the user at completion
• Whether to continue, particularly whether to avoid post actions when we have them
• An id and/or indicator of what caused the return code to be set
• Possibly more information to help with debugging

Alternatives (for example implicit conversion to an integer) should support the simplicity of one of these code variations:

public static void Main(string args)
{
   var cli = MyCreateCli();
   return cli.Invoke(args);
   return cli.Invoke(args).ReturnCode;

}

It would be highly desirable for this to include what to write to the console so the user can delay or collect output rather than having only the option to write to the console during action execution. For example, the CliAuthor may want to display progress as they proceed and then and then a summary at the end.

This struct could be part of the ParseResult. The parser plays an important role in the return since it offers return code and displays output to the user.

I am not suggesting that we go deep into the console problems prematurely, but the integer is not something we will be able to expand as we have richer options in the future.

[KalleOlaviNiemitalo]

Adding more information on the side of the exit code brings to mind dwServiceSpecificExitCode in SERVICE_STATUS — a well-intentioned feature but ultimately not supported in event log records written by the Service Control Manager, and I think rarely used by services.

[tmds]

Additional return values can always be added to the class that gets passed to the command handler later.

[KalleOlaviNiemitalo]

It would be highly desirable for this to include what to write to the console

That gets complex as some things need to be written to Console.Out and others to Console.Error (or to the equivalents in CliConfiguration) but the order must be preserved.

Also, errors and warnings might have to be in specific formats, like MSBuild and Visual Studio format for diagnostic messages or SARIF. For those, I have found it useful to define a diagnostic writer class and pass an instance of that to the command handler. Currently, my apps create those instances within what would now be CliAction.Invoke, and do not associate the instances with InvocationContext or CommandLineConfiguration.

[KathleenDollard]

@tmds Can you clarify what you mean?

Currently ParseResult is passed to a CliAction (ParseResult is where InvocationContext was in ICommandHandler). Do you mean that ParseResult could have rich information about the invocation results?

That could work, but I do not think it would be difficult to find. A return object with a ReturnCode property would make it quite obvious that what other stuff could be used.

A case could be made that since ParseResult is already there having rich return values on it was easier to pipe through. But the most key thing provided by a CliAction (old ICommandHandler implementation) is whether any additional processing is done. That is not strictly aligned with exit code. For example, clean-up operations could be needed or not needed in a post-action. It seems weird to me to pass a value that is eventually returned to the user around as though it was the key value, and then need another way to indicate continuation.

But there are many ways to implement this and I'm just concerned that we provide basics now, and that it be discoverable. I do not want us to have to return to the fundamentals of execution. We've stressed a lot about dropping the middleware pipeline in terms of not knowing how people were using it. Once we are in GA, we will be unable to make any changes without high confidence that we will not break folks.

[KathleenDollard]

@KalleOlaviNiemitalo Yes, it gets complicated.

I do not think we would ever block direct console writes, so where things need to appear in order can be handled. However, it is relatively annoying to create a summary block at the end of execution today. I think that should be quite simple. It could be used to summarize (the current output of dotnet build) or, depending on how we design it, it could be used to manage multiple concurrent operations having a sane output.

Mostly we are hands off the output space as there is so much need and opportunity here it needs a later focused effort. But I would like to lay some groundwork now.

[adamsitnik]

for example we could have a type that could be implicitly converted to int.

I believe that it would introduce "magic" that would be hard to understand, not to mention the increased complexity.

Imagine that I want to provide an action for my command. As an end user, I would need to become familiar with one more type or two (a base abstract class and a non-abstract that I could instantiate and return) or somehow discover that I can still return an int and it will somehow work.

A message to display to the user at completion

The completions use following method and whoever wants to provide more information for the completions can override it.

public class CliCommand
{
    public override IEnumerable<CompletionItem> GetCompletions(CompletionContext context)
}

My understanding is that a command is not invoked when we are asked for completions.

Whether to continue, particularly whether to avoid post actions when we have them

We can simply request the cancellation: #2157 (comment)

An id and/or indicator of what caused the return code to be set

Why not just return a specific code for each different scenario that we care about?

the integer is not something we will be able to expand as we have richer options in the future.

That is very true and I acknowledge that. I just don't see how to make it simple and intuitive at the same time.

[jonsequitur]

the integer is not something we will be able to expand as we have richer options in the future.

It's straightforward to provide custom SetAction overloads for other "return types". This was intentional in the design and I just spent a few minutes prototyping to get a sense of how well it might work.

The following works with no modifications to the current System.CommandLine API:

var command = new CliRootCommand();

command.SetAction(async (parseResult, cancellationToken) =>
{
    return new MyCustomResultType();
});

var config = new MyCustomConfiguration(command);

await config.InvokeAsync("x -y 123");

The custom extension method in this ex ample stores the return value on the custom CliConfiguration-derived type MyCustomConfiguration. This is probably sufficient surface area to built more opinionated framework-level APIs on without having to add complexity to the current CliAction to accommodate other return types. (Storing state on the configuration object is probably strange but we're still working on improving the design of that clas and CliRootCommand in any case.)

Interestingly, there's one piece missing here. Assuming I want to format the return value or parts of it for display, I need the ability to run some kind of post-action. This was achievable in middleware and an equivalent capabiltiy for CliAction is under discussion. (#2161)

[tmds]

(Storing state on the configuration object is probably strange but we're still working on improving the design of that clas and CliRootCommand in any case.)

fwiw, I think InvocationContext was a good fit for this.

[jonsequitur]

It was, and that was the intent. That seems to align more to the idea of an app framework and so we dropped it to reduce complexity in the core library.

And the idea of mutating the configuration (even a custom one) seems surprising.

The question would be then, where does this missing concept go?

Here's another strawman:

public class ParseResult
{
    public CliActionResult { get; set; }
}

public class CliActionResult
{
    public int ReturnCode { get; set; }
}

The idea would be that you could provide custom implementations and bring your own binding logic. A default post-action would set the return code based on whether an exception was caught. You could provide custom post-actions (rich display outputs) that act on your own custom properties (maybe bound from the ParseResult by your CliAction code).

Note that I'm deliberately avoiding conflating parsing and invocation.

[KalleOlaviNiemitalo]

ConditionalWeakTable<TKey, TValue> is also available for attaching data to ParseResult, but I guess it would be slower.