Move CompletionSourceList extension methods to CompletionSourceList

[jonsequitur]

    // All should move to instance methods.
    public static class CompletionSourceExtensions 
    {
        public static void Add(this CompletionSourceList completionSources, Func<CompletionContext,IEnumerable<string>> complete);
        public static void Add(this CompletionSourceList completionSources, CompletionDelegate complete);
        public static void Add(this CompletionSourceList completionSources, string[] completions);
    }
 

[adamsitnik]

Currently we have three different ways of adding completions:

1. Via CompletionSourceExtensions which currently can be used only with argument (nothing else exposes a collection of completions): arument.Completions.Add(complete).
2. Via ArgumentExtensions which delegates to CompletionSourceExtensions:

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

Line 28 in e965ae2

argument.Completions.Add(values);

but returns a reference to Argument to allow for chaining of fluent API calls:

var argument = new Argument<string>()
    .AddCompletions(expectedSuggestions);

3. Via OptionExtensions which also delegate to CompletionSourceExtensions:

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

Line 48 in e965ae2

option.Argument.Completions.Add(values);

and also returns the reference to Option so it can be used for chaining.

If we wanted to give up on chaining, we could have a single instance method on Argument:

argument.AddCompletions(x);
option.Argument.AddCompletions(x);

but we most likely don't want that. I think that we should just remove CompletionSourceExtensions and make Option.AddCompletions just delegate to Argument.AddCompletions.

@jonsequitur thoughts?

[jonsequitur]

Since completions can also be cleared, would it be clearer to have all completion-related functionality on a Completions property, e.g. Option.Completions.Add(x)?

Note that Option.Argument is internal so we'd want a way to access completions directly via Option in any case. (See #1884).

[adamsitnik]

remove CompletionSourceExtensions

I can see that doing this would break some stuff. The following syntax:

command-line-api/src/System.CommandLine.Tests/CompletionTests.cs

Lines 607 to 610 in b8ddb00

	new Argument<string>
	{
	Completions = { _ => new[] { "vegetable", "mineral", "animal" } }
	}

would stop working, as it maps to:

Argument<string> argument = new Argument<string>();
argument.Completions.Add((CompletionContext _) => new string[3] { "vegetable", "mineral", "animal" });

to get it working we would need to specify the types in explicit way:

new Argument<string>
    {
        Completions = { _ => new[] { new CompletionItem("vegetable"), new CompletionItem("mineral"), new CompletionItem("animal") } }
    }

[adamsitnik]

have all completion-related functionality on a Completions property

It would, but we would lose the fluent API syntax with chaining (assuming we would remove the extension methods and added a new property to Option)

[jonsequitur]

Fluent APIs aren't the norm in the System namespace anyway (though people do ask for them often.)

[adamsitnik]

We have removed CompletionSourceList in #1946 and we are now using ICollection<Func<CompletionContext, IEnumerable<CompletionItem>>>, so it's impossible to move these extensions methods anywhere. But they allow for using following C# syntax:

new Argument<DayOfWeek>
{
    Completions = { "mon", "tues", "wed", "thur", "fri", "sat", "sun" }
}

But... I am not sure if this feature is discoverable as Completions is not a collection of strings and seeing this work is honestly surprising to me. Also, AcceptOnlyFromAmong seems to be the best choice for such scenarios, since it does not only provide completion suggestions but also validates the input.

@jonsequitur @jozkee What do you think about removing these extensions methods and promoting AcceptOnlyFromAmong instead?

[KalleOlaviNiemitalo]

That syntax creates a separate one-element array and completion source for each string, bloating the IL somewhat. SharpLab

Doubling the braces like Completions = { { "mon", "tues", "wed", "thur", "fri", "sat", "sun" } } simplifies the IL quite a lot. SharpLab

I think I'd prefer removing the params and requiring callers to explicitly create the array Completions = { new[] { "mon", "tues", "wed", "thur", "fri", "sat", "sun" } }. SharpLab

[jonsequitur]

I think I'd prefer removing the params and requiring callers to explicitly create the array

I agree.

[adamsitnik]

I think I'd prefer removing the params and requiring callers to explicitly create the array

This does not answer my question:

What do you think about removing these extensions methods and promoting AcceptOnlyFromAmong instead?

AcceptOnlyFromAmong sets the right completions but it also verifies that parsed values belong to the set of legal values.

[jonsequitur]

Completions and validations are often orthogonal. For example, a completion source might contain a set of legal values from the file system, but these might not be the only acceptable values.