Simplify custom parsers and default value factories

[adamsitnik]

From #2046 :

Argument<T> provides multiple ways of setting default value: as T, Func<T> and Func<ArgumentResult, T>.
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. We need to find a way of providing Func<ArgumentResult, T> without specifying whether it's a default value provider or just a custom parser.

It applies to both Argument<T> and Option<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)

[adamsitnik]

My current best idea is following:

1. Make both Argument<T> and Option<T> have only a single ctor that always requires the name to be provided in explicit way (Providing name should be mandatory when creating Argument<T> and Option<T> #2051, Introduce ParseResult.GetValue<T>(string name) #2052).

public Argument(string name)
public Option(string name, params string[] aliases)

2. Option should accept a params string[] so users don't need to allocate an array in explicit way every time they create an Option.
3. Make other, optional information available to set and get via properties (Description etc).
4. When it comes to custom parser and default value factories, expose them also as properties:

public Func<ArgumentResult, T>? DefaultValueFactory { get; set; }
public Func<ArgumentResult, T>? ValueParser { get; set; }

By doing that we can:

• remove HasDefaultValue and HasCustomParser properties
• we don't need isDefault, as users can just assing both properties to the same method

PTAL at the example below and let me know how do you feel about it.

Details

using System.Collections;
using System.Runtime.InteropServices;

namespace SymbolCtors
{
    internal class Program
    {
        static void Main(string[] args)
        {
            // Sample: "dotnet build"
            Command dotnetBuild = new Command("build")
            {
                new Argument<FileInfo?>("file")
                {
                    Description = "The project or solution file to operate on. If a file is not specified, the command will search the current directory for one.",
                    DefaultValueFactory = (argumentResult) =>
                    {
                        FileInfo? first = new DirectoryInfo(Directory.GetCurrentDirectory()).EnumerateFiles("*.sln|*.proj").FirstOrDefault();

                        if (first is null)
                        {
                            argumentResult.AddError("Unable to find project or a solution file");
                        }
                        
                        return first;
                    }
                },
                new Option<bool>("ucr", "--ucr", "--use-current-runtime")
                {
                    Description = "Use current runtime as the target runtime."
                },
                new Option<string>("FRAMEWORK", "-f", "--framework")
                {
                    Description = "The target framework to build for. The target framework must also be specified in the project file."
                },
                new Option<Architecture>("arch", "-a", "--arch")
                {
                    Description = "The target architecture.",
                    ValueParser = (argumentResult) => Enum.Parse<Architecture>(argumentResult.Tokens[0]),
                    DefaultValueFactory = (_) => RuntimeInformation.OSArchitecture,
                },
                new Option<string>("os", "--os")
                {
                    Description = "The target operating system.",
                    ValueParser = GetOsName,
                    DefaultValueFactory = GetOsName,
                }
            };

            // an example of both a parser and a custom value provider
            static string GetOsName(ArgumentResult argumentResult)
            {
                if (argumentResult.Tokens.Count > 0)
                {
                    string provided = argumentResult.Tokens[0];

                    if (provided is "win" or "lin" or "osx")
                    {
                        return provided;
                    }

                    argumentResult.AddError($"Unknown Operating System: {provided}");
                }

                return OperatingSystem.IsWindows() ? "win" : OperatingSystem.IsLinux() ? "lin" : "osx";
            }
        }
    }

    public class Symbol
    {
        public string Name { get; }

        public string? Description { get; set; }

        private protected Symbol(string name)
        {
            Name = name;
        }
    }

    public class Argument<T> : Symbol
    {
        public Argument(string name) : base(name)
        {
        }

        public Func<ArgumentResult, T>? DefaultValueFactory { get; set; }
        public Func<ArgumentResult, T>? ValueParser { get; set; }
    }

    public class Option<T> : Symbol
    {
        private readonly Argument<T> _argument;

        public Option(string name, params string[] aliases) : base(name)
        {
            _argument = new Argument<T>(name);
        }

        public Func<ArgumentResult, T>? DefaultValueFactory
        {
            get => _argument.DefaultValueFactory;
            set => _argument.DefaultValueFactory = value;
        }

        public Func<ArgumentResult, T>? ValueParser
        {
            get => _argument.ValueParser;
            set => _argument.ValueParser = value;
        }
    }

    public class Command : Symbol, IEnumerable<Symbol>
    {
        private readonly List<Symbol> _symbols;

        public Command(string name, string? description = null) : base(name)
        {

        }

        public void Add(Symbol symbol) => _symbols.Add(symbol);

        public IEnumerator<Symbol> GetEnumerator() => _symbols.GetEnumerator();

        IEnumerator IEnumerable.GetEnumerator() => _symbols.GetEnumerator();
    }

    public class ArgumentResult
    {
        public List<string> Tokens { get; } = new();

        public void AddError(string errorMessage) { }
    }
}

[jonsequitur]

Per my comment here, I think we can make the API around names and aliases more intuitive by not requiring option constructors to provide both an unprefixed "name" and a mostly-matching prefixed "alias".

For example, instead of this:

new Option<bool>("ucr", "--ucr", "--use-current-runtime")

why not this:

new Option<bool>(name: "--ucr", aliases: "--use-current-runtime")

The term alias really refers to alternative names, so I think, for example, "--ucr" should be the name and "--use-current-runtime" an (optional) alias, or vice versa. This is the way I think most people understand the concepts. The fact that Option.Name removes the prefix passed to the constructor's name parameter is, at this point, a bug. It's a holdover from the naming convention lookups which were intended to be able to match method parameter names where - and / are invalid characters.

[KalleOlaviNiemitalo]

How do hidden aliases (#1859) fit into this API?

[adamsitnik]

@jonsequitur big thanks for a very detailed answer, it was very helpful.

I think that my understanding of what Option.Name really is (or what it was supposed to be originally) was wrong because:

1. Unintuitive prefix removal, using the Name only in Help and when creating parse errors, but not for parsing as the main ID.
2. Always adding Name to the collection of aliases and treating it also as an alias (it makes some parts of the code simpler).

Should separate Name and Aliases concepts, namely don't include Name in Aliases collection?

It would simplify the design of symbol types, as we would not need to check whether given alias can be removed in RootCommand.RemoveAlias:

command-line-api/src/System.CommandLine/RootCommand.cs

Lines 61 to 67 in 1e15ab1

	private protected override void RemoveAlias(string alias)
	{
	if (!string.Equals(alias, ExecutableName, StringComparison.Ordinal))
	{
	base.RemoveAlias(alias);
	}
	}

and could be expressed as a single property, which would be a step forward removing IdentifierSymbol type.

public string Name { get; }

+ public HashSet<string> Aliases { get; }
- public IReadOnlyCollection<string> Aliases { get; }
- public void AddAlias(string alias)
- private protected virtual void RemoveAlias(string alias)
- public bool HasAlias(string alias)

We would also not need to allocate a hashset of strings for symbols with no aliases (perf).
We could also easily differentiate the name and aliases.

And it would make it way easier to introduce hidden aliases, as it would be just a single new property:

public HashSet<string> HiddenAliases { get; }

We could also use a very fancy C# syntax:

new Option<bool>("--help", "-h")
{
    Description = "Show command line help.",
    HiddenAliases = { "-?", "help" }
}

The downside is that all the consumer of Aliases property would need to take this into account:

+ tokens.Add(command.Name, new Token(command.Name, TokenType.Command, command, Token.ImplicitPosition));

foreach (string commandAlias in command.Aliases)
{
    tokens.Add(
        commandAlias,
        new Token(commandAlias, TokenType.Command, command, Token.ImplicitPosition));
}

But to be honest I don't expect 99% of our users to be hit by that change.

I looked at the comment you mentioned and it includes a sample like this:

new Option<bool>("ucr", "--ucr", "--use-current-runtime")

You are right, this example is wrong, it should not work in that way.

It's wrong because I've tried to replace ArgumentHelpName and Name with just Name without having good understanding of how ArgumentHelpName is used by the customers.

After looking at more SDK command help examples I am convinced that we need to keep HelpName.

Arguments:
  <PROJECT | SOLUTION>  The project or solution file to operate on. If a file is not specified, the command will search
                        the current directory for one.

Setting Argument.Name to "PROJECT | SOLUTION" feels weird to me.
I would just rename it to HelpName from ArgumentHelpName and make it a part of every Symbol type.

Updated proposal:

Details

using System.Collections;
using System.Runtime.InteropServices;

namespace SymbolCtors
{
    internal class Program
    {
        static void Main(string[] args)
        {
            // Sample: "dotnet build"
            Command dotnetBuild = new Command("build")
            {
                new Argument<FileInfo?>("file")
                {
                    Description = "The project or solution file to operate on. If a file is not specified, the command will search the current directory for one.",
                    HelpName = "PROJECT | SOLUTION",
                    DefaultValueFactory = (argumentResult) =>
                    {
                        FileInfo? first = new DirectoryInfo(Directory.GetCurrentDirectory()).EnumerateFiles("*.sln|*.proj").FirstOrDefault();

                        if (first is null)
                        {
                            argumentResult.AddError("Unable to find project or a solution file");
                        }
                        
                        return first;
                    }
                },
                new Option<bool>("--use-current-runtime", "--ucr")
                {
                    Description = "Use current runtime as the target runtime."
                },
                new Option<string>("--framework", "-f")
                {
                    Description = "The target framework to build for. The target framework must also be specified in the project file.",
                    HelpName = "FRAMEWORK",
                },
                new Option<Architecture>("--arch", "-a")
                {
                    Description = "The target architecture.",
                    HelpName = "arch",
                    ValueParser = (argumentResult) => Enum.Parse<Architecture>(argumentResult.Tokens[0]),
                    DefaultValueFactory = (_) => RuntimeInformation.OSArchitecture,
                },
                new Option<string>("--os")
                {
                    Description = "The target operating system.",
                    HelpName = "os",
                    ValueParser = GetOsName,
                    DefaultValueFactory = GetOsName,
                },
                new Option<bool>("--help")
                {
                    Description = "Show command line help.",
                    // just to show that this syntax would be allowed if Aliases was a mutable collection wiith .Add method
                    Aliases = { "-h", "-?" }
                }
            };

            // an example of both a parser and a custom value provider
            static string GetOsName(ArgumentResult argumentResult)
            {
                if (argumentResult.Tokens.Count > 0)
                {
                    string provided = argumentResult.Tokens[0];

                    if (provided is "win" or "lin" or "osx")
                    {
                        return provided;
                    }

                    argumentResult.AddError($"Unknown Operating System: {provided}");
                }

                return OperatingSystem.IsWindows() ? "win" : OperatingSystem.IsLinux() ? "lin" : "osx";
            }
        }
    }

    public class Symbol
    {
        public string Name { get; }

        public string? Description { get; set; }

        public string? HelpName { get; set; }

        private protected Symbol(string name)
        {
            Name = name;
        }
    }

    public class Argument<T> : Symbol
    {
        public Argument(string name) : base(name)
        {
        }

        public Func<ArgumentResult, T>? DefaultValueFactory { get; set; }
        public Func<ArgumentResult, T>? ValueParser { get; set; }
    }

    public class Option<T> : Symbol
    {
        private readonly Argument<T> _argument;
        private HashSet<string>? _aliases, _hiddenAliases;

        public HashSet<string> Aliases => _aliases ??= new(StringComparer.Ordinal);

        public HashSet<string> HiddenAliases => _hiddenAliases ??= new(StringComparer.Ordinal);

        // Using "params" to avoid forcing users to allocate a new array
        //
        // An alternative is having a ctor with optional arguments
        // public Option(string name, string? firstAlias = null, string? secondAlias = null) : base(name)
        public Option(string name, params string[] aliases) : base(name)
        {
            _argument = new Argument<T>(name);

            _aliases = new HashSet<string>(aliases, StringComparer.Ordinal);
        }

        public Func<ArgumentResult, T>? DefaultValueFactory
        {
            get => _argument.DefaultValueFactory;
            set => _argument.DefaultValueFactory = value;
        }

        public Func<ArgumentResult, T>? ValueParser
        {
            get => _argument.ValueParser;
            set => _argument.ValueParser = value;
        }
    }

    public class Command : Symbol, IEnumerable<Symbol>
    {
        private readonly List<Symbol> _symbols;
        private HashSet<string>? _aliases, _hiddenAliases;

        public HashSet<string> Aliases => _aliases ??= new(StringComparer.Ordinal);
        public HashSet<string> HiddenAliases => _hiddenAliases ??= new(StringComparer.Ordinal);

        public Command(string name, string? description = null) : base(name)
        {
            Description = description;
        }

        public void Add(Symbol symbol) => _symbols.Add(symbol);
        public IEnumerator<Symbol> GetEnumerator() => _symbols.GetEnumerator();
        IEnumerator IEnumerable.GetEnumerator() => _symbols.GetEnumerator();
    }

    public class ArgumentResult
    {
        public List<string> Tokens { get; } = new();

        public void AddError(string errorMessage) { }
    }
}