Separate parse from invocation configurations

[jonsequitur]

This PR's goal is to separate the CommandLineConfiguration class into two separate classes, one of which configures parsing behaviors (now called ParserConfiguration) and the other of which configures invocation behaviors (InvocationConfiguration). InvocationConfiguration has four properties which were moved over from CommandLineConfiguration:

• EnableDefaultExceptionHandler
• ProcessTerminationTimeout
• Output
• Error

I've also removed the Parse, Invoke, and InvokeAsync methods from the former CommandLineConfiguration, since they're also available on ParseResult. The redundancy seemed unnecessary, although these invoke methods did include a Parse call, making the calling pattern more like the old, more succinct Command.Invoke methods.

Next steps:

• In light of these changes, I think both of these classes need better names.
• I think that passing a RootCommand to the CommandLineConfiguration constructor should not be necessary, and can lead to some unclear situations, such as:

var rootCommand = new RootCommand
{
    new Option<string>("-x")
};

var result = rootCommand.Parse(
    "-x which-root-command-controls-the-parser",
    new CommandLineConfiguration(
        new RootCommand
        {
            new Option<string>("-y")
        }));

[KalleOlaviNiemitalo]

I was going to comment that a suggest directive or a help action would need to be able to read the parsing configuration at invocation time, such as EnablePosixBundling. But I see ParseResult does have public properties for both configurations.

[adamsitnik]

Overall it LGTM, but please see my comments with some suggestions for improvements. Thank you @jonsequitur !

[adamsitnik]

For Parse methods, the configuration argument is optional (null by default, mapped into default config by the implementation). Perhaps Invoke should follow the same patter for consistency?

[jonsequitur]

Or should we change Parse to this overload approach versus coalescing the null parameter at runtime?

[jonsequitur]

If consistency is important here, another reason we might consider consolidating by splitting out Parse into two overloads (Parse() and Parse(ParserConfiguration)) is that that would not be a code-breaking change. Invoke has an InvokeAsync variant and this already has an optional CancellationToken argument, so adding an overload avoids breaking existing callers, since the InvocationConfiguration parameter comes before the CancellationToken parameter. (Of course there are a lot of breaking changes here so maybe this doesn't matter.)

[adamsitnik]

Or should we change Parse to this overload approach versus coalescing the null parameter at runtime?

I strongly believe that we should not do that. IIRC this shape was approved in the API Design, it would be another breaking change and also another public API.

Last, but not least my goal with exposing Parse(CommandLineConfiguration? config = null) as the only overload was to make it easier for the end users to discover the fact that CommandLineConfiguration exists (in the past, it was quite hard to find out how to customize parsing: you would need to know about the existance of builder type etc).

[adamsitnik]

Invoke has an InvokeAsync variant and this already has an optional CancellationToken argument, so adding an overload avoids breaking existing callers, since the InvocationConfiguration parameter comes before the CancellationToken parameter. (Of course there are a lot of breaking changes here so maybe this doesn't matter.)

That is very good point. However, from my observation it seems that users hardly ever provide CancellationToken to InvokeAsync as they usually call it from Main where they don't have any token.

[adamsitnik]

I almost forgot about #1909. Should we simply remove this custom exception type and throw something defined by the BCL?

[jonsequitur]

I had the same thought. I'd love suggestions on what would be an appropriate exception type.

[jonsequitur]

@KalleOlaviNiemitalo has a very good breakdown here: #1909 (comment)

[adamsitnik]

InvalidOperationException would be my best bet for now.

Or... even just removing ThrowIfInvalid? We don't use it anywhere outside of tests and it can be implemented with the public APIs we provide. It's also a bit confusing that it's part of the Config class, while what it does it checks the symbol hierarchy (and RootCommand is no longer part of the config)

[jonsequitur]

I like this suggestion. This method has always been awkward.

We used to do this validation at runtime. Then we decided it was too performance-intensive so we said it should be used only in tests. My worry has been that people will assume the parser will catch these kinds of mistakes automatically, and it's not intuitive to implement some of the checks. (And I'm pretty sure we missed some cases.) Of course, ASP.NET left the analogous issue (routing validation) as an exercise for the user, so probably System.CommandLine should do the same.