Add generic host builder pattern [WIP]

[NikiforovAll]

Hi @jonsequitur,
Here is my take on it. What do you think? Is it the right direction?

Closes #918

[NikiforovAll]

Currently, I'm looking for the recommended way to add RootCommand to already instantiated CommandLineBuilder. 🤔

CommandLineHost.CreateDefaultBuilder()
    .ConfigureCommandLineDefaults((CommandLineBuilder builder) =>
    {
        builder.AddCommand(rootCmd);
    })

[KathleenDollard]

@NikiforovAll you probably figured most of this out, but for anyone else.

Since the CommandLineBuilder is created before configuration, it already has a root command with the default (exe, help, version). AddCommand adds what you intend as a subcommand, which is not what is intended.

I see two solutions. @jonsequitur needs to weigh in:

1 Create a 'ReplaceRootCommand' method that wipes out everything except configuration in the builder and forces the new root command, or otherwise be able to set the RootCommand after the builder is created. (See Note below)
1 Instead of 'builder.AddCommand' here, configure the existing command

I think we should only do the first if we need to, but I think we need to. I'm just getting my head around this, but isn't this code likely:

CommandLineHost.CreateDefaultBuilder()
    .ConfigureCommandLineDefaults((CommandLineBuilder builder) =>
    {
        builder.SomeConfigurationChangeThatAffectsRootCommandCreation();
        builder.AddCommand(rootCmd);
    })

Note It isn't exactly this simple because creating the root prior to configuring the builder means the root isn't configured correctly. So I think we may need a pattern like:

CommandLineHost.CreateDefaultBuilder()
    .ConfigureCommandLineDefaults((CommandLineBuilder builder) =>
    {
        builder.SomeConfigurationChangeThatAffectsRootCommandCreation();
        builder.RecreateRoot();
        ConfigureRoot(builder.Command);
    })

Where ConfigureRoot is either separate or inline code, but is where you add your options/subcommands.

In StarFruit, I would expect this to be in a library method for all defaults, or when there is configuration of either StarFruit or System.CommandLine, it would look like:

CommandLineHost.CreateDefaultBuilder()
    .ConfigureCommandLineDefaults((CommandLineBuilder builder) =>
    {
        builder.SomeConfigurationChangeThatAffectsRootCommandCreation();
        builder.RecreateRoot();
        ConfigureCommand<T>();
    })

Of course, someone may have a more clever idea.

[jonsequitur]

It makes sense to allow the CommandLineBuilder to have an empty constructor and specify the command later, and it's very doable. The builder became smaller and smaller over time as we moved to mutable commands, options, and arguments, and what's left there hasn't changed in a long time, but that's not a reflection of the design having improved. Let's fix it.

[KathleenDollard]

CommandBuilder has room for an empty constructor. Would you then suggest a SetCommand method? (as opposed to a mutable Command)

RootCommandBuilder looks like it would take a breaking change, but if SetCommand is present and cleans out the builder, maybe it doesn't matter. Unless SetCommand should fail if Command is not null.

    public class CommandLineBuilder : CommandBuilder
    {
        private readonly List<(InvocationMiddleware middleware, int order)> _middlewareList = new List<(InvocationMiddleware middleware, int order)>();

        public CommandLineBuilder(Command? rootCommand = null)
            : base(rootCommand ?? new RootCommand())
        {
        }

[jonsequitur]

Something like this would probably match the existing patterns for these things:

builder.UseRootCommand(new RootCommand())

This way it's still chainable and you could also, if you wanted, put the whole command structure in there:

commandLineBuilder.UseRootCommand(new RootCommand
{
  new Command("apple"),
  new Command("banana")
})

[KathleenDollard]

There are some other issues I'd like to look at later, but the problematic code is:

       public static async Task CommandLineHost_creates_host_for_simple_command()
        {
...
            await CommandLineHost.CreateDefaultBuilder()
                .ConfigureCommandLineDefaults((CommandLineBuilder builder) =>
                {
                    // TODO: it is not possible to add it like this atm.
                    builder.AddCommand(rootCmd);
                })
                .Build()
                .RunAsync(tokenSource?.Token ?? default);

The builder has a command at this point. The philosophy is that the builder always has a command. So, does the following work:

           await CommandLineHost.CreateDefaultBuilder()
                .ConfigureCommandLineDefaults((CommandLineBuilder builder) =>
                {
                    // The following mutates the existing command, rather than add a new one
                    builder.Command.AddOption(myOption);
                })
                .Build()
                .RunAsync(tokenSource?.Token ?? default);

This is what I have in mind for StarFruit (with whatever name we land on)

           await CommandLineHost.CreateDefaultBuilder()
                .ConfigureCommandLineDefaults((CommandLineBuilder builder) =>
                {
                    // The following mutates the existing command, rather than add a new one
                    builder.Command.ConfigureStarFruit<MyCLIModelRoot>();
                })
                .Build()
                .RunAsync(tokenSource?.Token ?? default);

I believe everything within the command is mutable, although replacing the command in the builder is not possible, for reasons that I think remain valid.

Do you see any issues with this approach?

[NikiforovAll]

I think using the mutability of a Command is a good thing. Why don't we use this on the builder directly?
builder.ConfigureStarFruit<MyCLIModelRoot>();

[KathleenDollard]

I am out of time this morning, but this is how far I got on your test:

• Host is null in the handler for reasons I didn't debug
• The approach you used for testing the options doesn't work

      [Fact]
        public static async Task CommandLineHost_creates_host_for_simple_command()
        {
            //Arrange
            // var args = new string[] { $"--foo", "42" };
            MyOptions options = null;
            IHost hostToBind = null;
            var check = 0;

            var option = new Option($"--foo") { Argument = new Argument<int>() };
            var handler = CommandHandler.Create<IHost>((host) =>
           {
               hostToBind = host;
               check = 7;
           });
            // Act
            // var tokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(1));
            CancellationTokenSource tokenSource = null;
            await CommandLineHost.CreateDefaultBuilder()
                .ConfigureCommandLineDefaults((CommandLineBuilder builder) =>
                {
                    builder.Command.AddOption(option);
                    builder.Command.Handler = handler;
                })
                .Build()
                .RunAsync(tokenSource?.Token ?? default);
            // Assert
            //Assert.NotNull(hostToBind);
            //Assert.Equal(42, options.MyArgument);
            Assert.Equal(7, check);
        }

[NikiforovAll]

@KathleenDollard Sorry for not communicating the status of current PR. Currently, I'm waiting for design and final thoughts in #918 (+ #918 (comment))

This happens because it is only possible to access InvocationContext from UseMiddleware method. And the method you see in PR is just a dummy that is not used during actual execution of a handler (ref: src\System.CommandLine.Hosting\DefaultBuilder\GenericCommandLineHostBuilder.cs:34)

I stumbled on the cyclic dependency between CommandLineBuilder and IHostBuilder:
CommandLineBuilder depends on IHost and to build IHost we need to provide ParseResult, BindingContext etc. to IHost . (At least, it is like that at the moment).

I see two possible solutions:

1. Separate the parsing process and execution process. Expose invocation context as a result of the parsing process and add to DI

// step 1
Parser parser = commandLineBuilder.Build();
ParseResult parseResult = parser.Parse(Args);
// access invocation from parseResult
host.ConfigureServices(services =>
{
    services.TryAddSingleton<InvocationContext>(invocation);
    //...
});
// step 2 inject IHost to CommandLineBuilder right before execution 
// src\System.CommandLine.Hosting\DefaultBuilder\CommandLineExecutorService.cs:36
public async Task StartAsync(CancellationToken cancellationToken)
{
    invocation.BindingContext.AddService(typeof(IHost), _ => host);
    //...
    await parseResult.InvokeAsync();
    appLifetime.StopApplication();
}

2. Build CommandLineBuilder one more time in HostedService via UseMiddleware(). I think it might introduce a lot of bugs.