Symbols: Validators and Completions

[adamsitnik]

Based on various recent discussions with @jozkee and @KalleOlaviNiemitalo

#1953 (comment)
https://github.com/dotnet/command-line-api/pull/1959/files#r1025309941

I've realized that:

• we currently don't expose a possibility to remove Completions from Option<T> (Argument exposes .Completions property, but Option does not). Most of the methods exposed by Option simply delegate to .Argument which is an internal property.
• there is more than one way of adding new completions and validators: symbol.AddValidator and symbol.Validators.Add

The questions I have:

• should we simplify the public API surface by just exposing the mutable collections and removing Add* methods? @bartonjs what is the recommended approach for such scenarios? The change I am talking about:

public class Argument<T>
{
    public ICollection<Func<CompletionContext, IEnumerable<CompletionItem>>> Completions { get; }
-   public Argument<T> AddCompletions(Func<CompletionContext, IEnumerable<CompletionItem>> completionsDelegate)
// we don't expose ClearCompletions()
}

• should we remove the Option methods that just delegate to Argument and expose Option.Argument? @jonsequitur this has most likely been discussed in the past, what is the main reason for not exposing this property?

public class Option<T>
{
+   public Argument<T> { get; }
-    public void SetDefaultValue(T value);
-    public void SetDefaultValueFactory(Func<T> defaultValueFactory)
-    public Option<T> AcceptOnlyFromAmong(params string[] values)
-    public Option<T> AddCompletions(params string[] completions)
-    public Option<T> AddCompletions(Func<CompletionContext, IEnumerable<string>> completionsDelegate)
-    public Option<T> AddCompletions(Func<CompletionContext, IEnumerable<CompletionItem>> completionsDelegate)
-    public Option<T> AcceptLegalFilePathsOnly()
-    public Option<T> AcceptLegalFileNamesOnly()
}

It would simplify the public API surface and we would avoid the issues where not every method exposed by Argument is exposed by Option, but at a cost of losing the fluent API builder capabilities.

[adamsitnik]

A different solution we have discussed offline was to make Option<T> derive from Argument<T>. I am going to write a simple prototype and verify how good/bad this idea is.

[KalleOlaviNiemitalo]

@adamsitnik would that change affect the tokens in OptionResult and ArgumentResult?

[adamsitnik]

A different solution we have discussed offline was to make Option derive from Argument

I've given it a try and it's not worth it.

Advantages:

• public API simplification for Option<T>
• no need to sync Argument and Option methods
• a parent of every Symbol could be only a Command (currently every Argument owned by an Option needs to add Option as its parent)

Disadvantages:

• Argument does not derive from IdentifierSymbol, but Option and Command do. Since we have no multiple class inheritance, the IdentifierSymbol APIs would need to be duplicated for Option<T> and Command.
• lack of non-generic Option type that is required to store options of different kinds that belong to a single Command instance. This could be worked around by having an Option interface, but it would also need to be public so it would defeat our purpose (fewer public types and APIs). Moreover, Option interface would never derive from Symbol type, so we would need to cast Option to Symbol in multiple places.
• fluent Option<T> methods would always return an Argument<T>, which would make it hard to use as an option later

We could achieve multiple class inheritance by changing Option and IdentifierSymbol to interfaces with default implementations, but this would not work with older .NET Runtimes.

The lists are not complete as I did not want to invest too much time working on it.