More design questions

[adamsitnik]

We are soon going to have another internal round of design discussion for System.CommandLine. I am sharing the list of open questions here so everyone can share their feedback.

Symbols

Symbol

Description

Symbol.Description does not need to be virtual: #2045

Every symbol can provide the description in the ctor.

Argument

HelpName

When Argument is parsed, we use its position rather than name. But when we want to refer to it, for example when displaying help, it may need a name.

Currently the type has both Name and HelpName. Why does Argument need both?

Arity

Can anyone provide a better name?

Parse(string), Parse(string[])

Both methods are used only for testing purposes in System.CommandLine. They don't provide any possibility to customize the configuraiton used for parsing. They can be easily replaced with one liner:

new CommandLineBuilder(new Command() { argument }).UseDefaults().Build().Parse($input);

I believe that we should remove them or made them internal.

Validators

Currently a validator is an Action<ArgumentResult>. What I don't like about this design is the fact that it allows for modificaitons of the ArgumentResult.

Example:

argument.Validators.Add(argumentResult => argumentResult.OnlyTake(3));

It's hard to propose a better design, as some validators need more than just SymbolResult.Tokens. Example: validator may want to call GetValueOrDefault to validate parsed value.

Argument

ctors

Argument<T> provides multiple ways of setting default value: as T, Func<T> and Func<ArgumentResult, T>

public Argument(Func<T> defaultValueFactory)
    
public Argument(
    string name,
    T defaultValue,
    string? description = null)

public Argument(
    string? name,
    Func<ArgumentResult, T> parse,
    bool isDefault = false,
    string? description = null)

What I don't like is the fact that the last ctor requires the user to define whether given Func<ArgumentResult, T> is a default value provider or a custom parser. It's confusing.

My understanding is that we need custom parser for complex types and default values for all kinds of types. IMO the latter is way more common than the first.

Do we really need a default value provider that uses ArgumentResult as an input?
Do we really need the default value to be lazily provided?

How about having just one ctor:

public Argument(
    string? name = null,
    string? description = null
    T? defaultValue = null,
    Func<ArgumentResult, T> customParser = null)

SetDefaultValue, SetDefaultValueFactory

Same as above, there are 3 different ways of setting default value:

public void SetDefaultValue(T value)
public void SetDefaultValueFactory(Func<T> defaultValueFactory)
public void SetDefaultValueFactory(Func<ArgumentResult, T> defaultValueFactory)

I would stick with two:

public void SetDefaultValue(T value)
public void SetCustomParser(Func<ArgumentResult, T> customParser)

AcceptLegalFilePathsOnly, AcceptLegalFileNamesOnly

These instance methods are usefull only for argument of string, FileInfo and arrays of these two.

Currently they can be used for all kinds of T:

Argument<int> portNumber = new ();
portNumber.AcceptLegalFilePathsOnly();

Does anyone have a better idea how to make sure they don't get suggested/used for T other than the ones it makes sense for?

We could of course introduce them as extension method of Argument<string>, Argument<string[]>, Argument<FileInfo>, Argument<FileInfo[]>, but I doubt it would be accepted by BCL review board.

IdentifierSymbol

From my point of view the existence of this type is an implementation detail (it's a Symbol with Aliases).

I need to discuss this case with Jeremy.

Option

ArgumentHelpName

Similar to Argument.Name: the users have the possibility to set Name for Option. Do we really need this property? We use aliases for parsing and it seems like Name is exactly what we need for help.

IsGlobal

This property is currently internal, but we should expose it.

We need a better name. Ideas?

Parse(string), Parse(string[])

Same as Arugment.Parse: do we really need these methods to be public?

Option

ctors

Same as for Argument<T>: we need a better way to distinguish default value provider and custom parser.

Command

Command.Children

Command exposes Argument, Options, and Subcommands properties.

It also implements IEnumerable<Symbol> and Add(Symbol) to support a very common duck typing C# syntax:

Command cmd = new ()
{
    new Argument<int>() // uses duck typing and calls Command.Add(Symbol).
};

Do we really need to expose IEnumerable<Symbol> Children on top of that? It seems redundant to me.

AddGlobalOption

If we made Option.IsGlobal public, we can remove this method, so users can do:

command.Options.Add(new Option<bool>("some") { IsGlobal = true });

TreatUnmatchedTokensAsErrors

Does it make sense to make this setting specific to given command? Should it become a part of CommandLineConfiguration?

Parse

Command has no Parse instance methods, but it does have an extension method.

IMO we should make it an instance method with optional CommandLineConfiguration argument to make it easier to discover the configurability of parsing:

ParseResult Parse(CommandLineConfiguration? configuration = null)

RootCommand

Currently it exposes only two public methods:

public static string ExecutablePath => Environment.GetCommandLineArgs()[0];

public static string ExecutableName => Path.GetFileNameWithoutExtension(ExecutablePath).Replace(" ", "");

This information can be easily obtained by System.CommandLine users (by using System.Environment class).

Currently the only difference with Command is that it does not allow for setting custom alias (which should be ultra rare). Could this be achieved without a dedicated RootCommand type? I want to reduce the number of concepts the users need to get familiar with. Better perf (fewer types to load and JIT) is just a side effect of that,

Parsing

CommandLineConfiguration

This type provides multiple internal properites and some public properties. All of them are readonly. Should we make them mutable to avoid the need of having a builder type?

EnableEnvironmentVariableDirective, EnableSuggestDirective, EnableDirectives

If we expose all the internal properties, are we going to introduce a new property for every new directive?

What are the alternatives other than creating dedicated directive types and adding Command.Directives property?

EnableTokenReplacement

Why is it enabled by default?

TokenReplacer

Why do we need custom token replacers? What do users customize other than supporting comments in the response files?

Perhaps we should expose an enumeration and provide default implementation?

enum ResponseFiles
{
    Disabled,
    Enabled,
    SpecificStandardName1,
    SpecificStandardName2,
    
}

config.ResponseFiles = ResponseFiles.Disabled;

What I don't like about the current design is that InvokeAsync performs parsing which can perform a blocking call when reading the response file:

command-line-api/src/System.CommandLine/Parsing/StringExtensions.cs

Line 386 in 1bc03fe

var lines = File.ReadAllLines(filePath);

If we just provide an enumeration, it would be easier to do the right thing (call async APIs for async invocation, pass CancellationToken etc).

LocalizationResources

In BCL, System.* libraries that need localization provide their own set of resource files, configure automation with https://github.com/dotnet/arcade/blob/main/Documentation/OneLocBuild.md and don't expose a possibility to customize the transaltions. This type needs to be removed / made internal.

Please see #2041 for more details.

CommandLineBuilder

The type name is confusing to me. It builds a parser and configuration and I would expect it to be called CommandLineConfigurationBuilder. But perhaps we can remove it if we make CommandLineConfiguration mutable?

It defines almost no public methods, but CommandLineBuilderExtensions provides plenty of its extensions.

Properties are not always enough

Not every configuration can be expressed with a single property. Some methods allow for additional customization.

For example UseParseErrorReporting allows for enabling the parse error reporting and setting a custom exit code for parsing errors:

public static CommandLineBuilder UseParseErrorReporting(
    this CommandLineBuilder builder,
    int errorExitCode = 1)

RegisterWithDotnetSuggest

The current design has plenty of flaws. One of them is major perf hit for default config (creating files and starting new process to register).

We most likely need a separate meeting dedicated to dotnet suggest. Who should be invited?

UseDefaults

Before we find a proper solution for RegisterWithDotnetSuggest, can we disable it by default?

UseExceptionHandler

Curent API:

public static CommandLineBuilder UseExceptionHandler(
    this CommandLineBuilder builder,
    Action<Exception, InvocationContext>? onException = null,
    int? errorExitCode = null)

In my opinion exception handler should always return an int. It would make the config simpler and easier to discover the possibility to set custom exit code.

public static CommandLineBuilder UseExceptionHandler(
    this CommandLineBuilder builder,
    Func<Exception, InvocationContext, int>? onException = null);

AddMiddleware

Middleware is VERY powerfull. After #2043 we are close to having 0 middleware by default (it's expensive).

I need to make a research and see why our users provide custom middleware. Just to make sure it's not just a mechanism for workarounding our limitations like missing configuraiton etc.

UseHelp

I've not analysed this part of S.CL yet. I will have comments in the near future.

Parser

First of all I believe that this type is simply hard to discover. The only state it has is CommandLineConfiguration. To make CommandLineConfiguration more discoverable I would make Parser static with just one method:

static class Parser
{
    static PareResult Parse(string[] args, CommandLineConfiguration configuration = default);
}

We also need to rename it to CliParser to avoid conflicts with dozen other parsers ;)

ParseResult

Directives

Currently the only way for the users to implement custom directives is enabling directives parsing in the config and using this property to get parsed output.

The problem is that if the users want to perform any custom action based on the directive input like setting custom culture: [culture pl] they need to use Middleware (expensive).

Should we expose a dedicated Directive symbol type?

class Directive : Symbol
{
    Directive(string name, Action<string> onParsed);
}

UnmatchedTokens

The Tokens property returns a collection of Token type. UnmatchedTokens returns a collection of strings. Why don't we expose full information?

GetValue

Do we need both the generic and non-generic overloads?

object? GetValue(Option option) 
T? GetValue<T>(Option<T> option)
object? GetValue(Argument argument)
T GetValue<T>(Argument<T> argument)

SymbolResult

SymbolResult and it's entire family LGTM after recent changes.

[KalleOlaviNiemitalo]

Do we really need a default value provider that uses ArgumentResult as an input?
Do we really need the default value to be lazily provided?

I need access to the values of other command-line options in completions and validation, but apparently not in parsing and default values.

The main scenario for default values would be something like --target in MSBuild, which defaults to whatever is specified in the DefaultTargets attribute in the project file that can be specified as a command-line argument; but this can also be modelled as having null as the default and translating that in the command handler after parsing.

[KalleOlaviNiemitalo]

Currently the type has both Name and HelpName. Why does Argument need both?

My understanding is that Argument.Name is culture-invariant for binding against names from reflection, and Argument.HelpName is localised for help.

But, perhaps it would be better to have a string BindingName property dedicated to binding, not corresponding to a parameter of the constructor.

[KalleOlaviNiemitalo]

If we made Option.IsGlobal public, we can remove this method

Yes please. The current AddGlobalOption method mutates the Option by setting IsGlobal so it is not possible to add the same Option as a global option of one command tree and as a local option in another command. Making IsGlobal public would make this obvious rather than a surprise.

[KalleOlaviNiemitalo]

Directive(string name, Action<string> onParsed);

No, this presupposes that invocation is to continue after the directive. Which is not true for the [parse] and [suggest] directives and so likely would not be true for application-defined directives either.

[KalleOlaviNiemitalo]

Do we need both the generic and non-generic overloads [of ParseResult.GetValue]?

The generic methods are nice to use with type deduction; please keep them.

The non-generic methods might be useful for some reflection scenario where the caller does not know the type at compile time. However, if they were removed, then those applications could still call FindResultFor(argument_or_option)?.GetValueOrDefault<object?>() for the same effect. This would not convert the argument but then again the current non-generic methods don't do that either. So I think they are OK to remove.

[karlra]

Since there was a request for feedback on Twitter, here it is. I am using this library in production and while it's great and I am sure it is very powerful and can do a million things, in 99% of cases you just want to get the damn command line values, no fancy stuff. Maybe I am misunderstanding the library somehow but when I implemented it, what felt like it should be a simple task was not straightforward and that someone forgot about the most common scenario while designing for very complex scenarios.

It took quite some doing and a lot of code (that looks intimidating to a newbie) to understand how to arrive at a simple GetParsedValue< T >(paramName) way of using the library. It involved setting up a custom handler (a method that has 20 overloads), then use an "InvocationContext", whatever that is, to get to the actual parameters. If there is an easier way to achieve this way of using the library, it is sure hidden well.

Also, just a personal opinion, but the suggested way in the docs to bind parameters with SetHandler< T1, T2, T3.... >, in the beta version, supports just 8 parameters, and what happens when you need more than whatever the arbitrary limit is? Total rewrite!

Since a command line invocation is by definition a one-time occurrence, why isn't there a top-level, quick, discoverable way to just get a parsed parameter value by name, like System.CommandLine.Current.GetValue< T >(paramName)?

[patriksvensson]

Instead of Arity, perhaps Ordinal or Index is a better name?

[jonsequitur]

this can also be modelled as having null as the default and translating that in the command handler after parsing.

This is certainly something people can do, but the default is then unknowable without actually running the command. Experiences like validation, help messages, dry runs, completions, etc. are stronger if this is modeled in the parser layer.

[jonsequitur]

My understanding is that Argument.Name is culture-invariant for binding against names from reflection

This was the original intent but this implementation is no longer used in the core library.

[jonsequitur]

Should we expose a dedicated Directive symbol type?

I would tend to say yes but for invocation, use a common approach that could also potentially be applied to certain options that have "command-like" behaviors, such as --version and --help. Similar to directives like [parse] and [suggest], these indicate an alternative behavior where the rest of the command line input is still significant. We should assume these handlers need to access the InvocationContext just like command handlers do.

[KalleOlaviNiemitalo]

Instead of Arity, perhaps Ordinal or Index is a better name?

Ordinal and cardinal numbers… Cardinality?

[jonsequitur]

Also, just a personal opinion, but the suggested way in the docs to bind parameters with SetHandler< T1, T2, T3.... >, in the beta version, supports just 8 parameters, and what happens when you need more than whatever the arbitrary limit is? Total rewrite!

Many people share this opinion. We're going to remove those overloads and provide a source generator-based approach to simplify users' code. In the meantime, if you choose the InvocationContext overload, you can parse as many values as you like using InvocationContext.GetValue.