New Config proposal

[adamsitnik]

One of our goals is simplyfing the configuration, making it easier to discover and use.

This can be achieved by making the configuration mutable (no need for a dedicated builder) and making it an optional argument for Command.Parse (this has already happened).

My current idea is following:

public class CliConfiguration
{
    public CliConfiguration(Command command)
    {
        Command = command;
        Directives = new()
        {
            new SuggestDirective()
            // I am not sure whether we should enable other Directives by default
            // new EnvironmentVariablesDirective()
            // new ParseDirective()
        };
    }

    public Command Command { get; }

    public bool EnablePosixBundling { get; set; } = true;

    public bool EnableDefaultExceptionHandler {  get; set; } = true;

    public bool EnableTypoCorrections { get; set; } = false;

    public TimeSpan? ProcessTerminationTimeout { get; set; } = TimeSpan.FromSeconds(2);

    public int ParseErrorExitCode { get; set; } = 1;

    public TryReplaceToken? ResponseFileTokenReplacer { get; set; } = StringExtensions.TryReadResponseFile;

    public List<Directive> Directives { get; }
}

How to disable signaling and handling of process termination via a CancellationToken? Set ProcessTerminationTimeout to null.

How to disable default response file token replacer? Set ResponseFileTokenReplacer to null.

How to customize exception handler? Set EnableDefaultExceptionHandler to false and catch the exceptions on your own:

CliConfiguration config = new(command) { EnableDefaultExceptionHandler = false };
ParseResult parseResult = command.Parse(args, config);

try
{
    return parseResult.Invoke();
}
catch (Exception ex)
{
    // custom exception handler
}

The alternative for customizing the ParseErrorExitCode is following:

ParseResult parseResult = command.Parse(args);

if (parseResult.Errors.Any())
{
    return $customValue;
}

The most tricky part is how HelpOption and VersionOption should be enabled and disabled. Currently, the config builder just adds them to the provided Command options list:

command-line-api/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs

Lines 336 to 351 in 3b5c4be

	private static void OverwriteOrAdd<T>(Command command, T option) where T : Option
	{
	if (command.HasOptions)
	{
	for (int i = 0; i < command.Options.Count; i++)
	{
	if (command.Options[i] is T)
	{
	command.Options[i] = option;
	return;
	}
	}
	}
	command.Options.Add(option);
	}

I don't like it, as it's a side effect. But on the other hand, I don't have an ideal alternative.

Possible solutions:

Expect the users to add them in explicit way.

command.Options.Add(new HelpOption());
command.Options.Add(new VersionOption());

Advantage(s): no magic, everything is crystal clear, perf.
Disadvantage(s): help would be not enabled by default.

Create every command with Help and Version options added by default

public Command(string name, bool addDefaultOptions = true)
{
    if (addDefaultOptions)
    {
        Options.Add(new HelpOption());
        Options.Add(new VersionOption());
    }
}

Advantage(s): help enabled by default.
Disadvantage(s): Hard to customize help (find, cast & customize or replace) I expect that Version makes sense only for the root command, it would pollute subcommands.

Perhaps it would make more sense for RootCommand?
Maybe the argument should not be optional, so everyone who creates a Command would need to think about it?

Expose the options as part of Config type, add them the root Command when parsing

public class CliConfiguration
{
    public HelpOption? HelpOption { get; set; } = new ();
    
    public VersionOption? VersionOption { get; set; } = new ();
}

Advantage(s): quite easy to discover and configure, enabled by default.
Disadvantage(s): side effect of adding them to the root command (but so far nobody complained about it beside me?)

Do we really need VersionOption enabled by default?

Add a list of default Options to Config, similarly to Directives

public class CliConfiguration
{
    public List<Option> Options { get; }
}

Advantage(s): quite easy to discover, consistent with Directives
Disadvantage(s): hard to configure a specific option (find HelpOption, cast it and then set the builder), side effect of adding them to the root command

@jonsequitur @KathleenDollard @Keboo @KalleOlaviNiemitalo please provide feedback

[adamsitnik]

With the regards to IConsole, we should IMO extend the config in following way:

public class CliConfiguration
{
    /// <summary>
    /// The standard output.
    /// </summary>
    public TextWriter Out { get; set; } = Console.Out;

    /// <summary>
    /// The standard error.
    /// </summary>
    public TextWriter Error { get; set; } = Console.Error;
}

this would allow us to still test the apps without introducing new abstraction to BCL and remove the last property from InvocationContext (to replace InvocationContext with ParseResult everywhere)

[adamsitnik]

Feedback from @KalleOlaviNiemitalo #2103 (comment)

[KalleOlaviNiemitalo]

The alternative for customizing the ParseErrorExitCode is following:

ParseResult parseResult = command.Parse(args);

if (parseResult.Errors.Any())
{
    return $customValue;
}

I guess this would need some additional check so that HelpOption with parse errors shows help and ignores the parse errors. Didn't review the implementation yet.

[KalleOlaviNiemitalo]

Disadvantage(s): I expect that Version makes sense only for the root command, it would pollute subcommands.

Especially if the subcommand needs --version for a different purpose, like in dotnet add package [-v|--version <VERSION>].

[KalleOlaviNiemitalo]

Yeah I don't like Command.Parse having side effects on Command.Options. It seems cleaner if a multithreaded server can set up the Command tree once and then use that for parsing command lines from different clients on different threads; but I didn't check whether Symbol or Command already has some mutable caches that would make that unsafe.

[tmds]

Will the library provide something that combines CliConfiguration with a RootCommand in a single object, so a Main method could look like:

var commandLine = CreateAppCommandLine();
return await commandLine.InvokeAsync(args);

[jonsequitur]

Yeah I don't like Command.Parse having side effects on Command.Options.

Agreed. While the symbol types are mutable because it makes the API easier to use, System.CommandLine itself should not mutate them indirectly. This is why the implicit parser used by Command.Parse was both an internal implementation detail and a code smell.

[jonsequitur]

Will the library provide something that combines CliConfiguration with a RootCommand in a single object, so a Main method could look like:

I think CommandLineConfiguration is that type (since it carries a reference to a RootCommand), although maybe the name makes that unclear.

[tmds]

I think CommandLineConfiguration is that type (since it carries a reference to a RootCommand), although maybe the name makes that unclear.

Yes, the name is appropriate for something that holds configuration settings. Not for something that is invoked on.

[KalleOlaviNiemitalo]

After trying the result of #2102, I don't like how the configuration builder injects HelpOption and VersionOption to Command.Options. Instead I would like one of:

• CommandLineConfiguration.Options which is merged with Command.Options of the root command during Parse, similar to CommandLineConfiguration.Directives. Not sure what should happen if the same token strings are in both.
• Command.AddStandardOptions() which applications would call explicitly if they want --help and --version.

In one of my applications, there is a subcommand for which I want --help to output an additional section. After #2102, I implement that by first calling Parse, then checking for HelpAction, and checking whether the ParseResult.CommandResult.Command is that specific command; if so, then I call CustomizeLayout on the HelpBuilder that I get from the HelpAction. But this mutates the HelpBuilder that is referenced by HelpAction that is referenced by HelpOption that is referenced by the Command, so the Command is not deeply immutable after initialisation. It would be a bit nicer if the mutation were in CommandLineConfiguration or HelpContext only. There is a similar consideration with the width of the help output, I suppose.

[jonsequitur]

Not sure what should happen if the same token strings are in both.

Overriding global options' aliases on specific subcommands has always been supported, so this should work fine.

Command.AddStandardOptions() which applications would call explicitly if they want --help and --version.

We've tried to make sure that end users get a good CLI experience by default, so I would worry this would lead to a proliferation of command line tools without help support.

[adamsitnik]

An alternative solution that we have just discussed offline with @jonsequitur is extending RootCommand with all the configuration properties (things like Directives make sense only on root command level). We are going to give it a try and get back with our findings.

[adamsitnik]

I took one more look at it, here are my thoughts:

Feels natural for RootCommand:

public List<Directive> Directives { get; }

Is related to parsing, in theory could be customized per Command (moved even to Command):

public bool EnablePosixBundling { get; set; } = true;
public bool EnableParseErrorReporting { get; set; } = true;
public bool EnableTypoCorrections { get; set; } = false;
public TryReplaceToken? ResponseFileTokenReplacer { get; set; } = StringExtensions.TryReadResponseFile;

Is related to execution and is required to be configurable for scenarios where RootCommand is not used:

public bool EnableDefaultExceptionHandler { get; set; } = true;
public TimeSpan? ProcessTerminationTimeout { get; set; } = TimeSpan.FromSeconds(2);
public TextWriter Output { get; set; } = Console.Out;
public TextWriter Error { get; set; } = Console.Error;

@jonsequitur I lean towards keeping CliConfig, making it mutable and exposing Help and Version as two properties:

public class CliConfiguration
{
// parsing 
    public bool EnablePosixBundling { get; set; } = true;
    
    public bool EnableParseErrorReporting { get; set; } = true;

    public bool EnableTypoCorrections { get; set; } = false;
    
    public TryReplaceToken? ResponseFileTokenReplacer { get; set; } = StringExtensions.TryReadResponseFile;

// symbols
    public List<Directive> Directives { get; }

    public HelpOption? HelpOption { get; set; } = new ();
    
    public VersionOption? VersionOption { get; set; } = new ();
    
// execution
    public bool EnableDefaultExceptionHandler { get; set; } = true;
    
    public TimeSpan? ProcessTerminationTimeout { get; set; } = TimeSpan.FromSeconds(2);

    public TextWriter Output { get; set; } = Console.Out;

    public TextWriter Error { get; set; } = Console.Error;
}