Repository navigation
Move CompletionSourceList extension methods to CompletionSourceList #1910
Description
// 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);
}
Activity
adamsitnik commentedon Nov 3, 2022
Currently we have three different ways of adding completions:
CompletionSourceExtensionswhich currently can be used only with argument (nothing else exposes a collection of completions):arument.Completions.Add(complete).ArgumentExtensionswhich delegates toCompletionSourceExtensions:command-line-api/src/System.CommandLine/ArgumentExtensions.cs
Line 28 in e965ae2
but returns a reference to
Argumentto allow for chaining of fluent API calls:OptionExtensionswhich also delegate toCompletionSourceExtensions:command-line-api/src/System.CommandLine/OptionExtensions.cs
Line 48 in e965ae2
and also returns the reference to
Optionso it can be used for chaining.If we wanted to give up on chaining, we could have a single instance method on
Argument:but we most likely don't want that. I think that we should just remove
CompletionSourceExtensionsand makeOption.AddCompletionsjust delegate toArgument.AddCompletions.@jonsequitur thoughts?
jonsequitur commentedon Nov 3, 2022
Since completions can also be cleared, would it be clearer to have all completion-related functionality on a
Completionsproperty, e.g.Option.Completions.Add(x)?Note that
Option.Argumentis internal so we'd want a way to access completions directly viaOptionin any case. (See #1884).adamsitnik commentedon Nov 3, 2022
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
would stop working, as it maps to:
to get it working we would need to specify the types in explicit way:
adamsitnik commentedon Nov 3, 2022
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 commentedon Nov 3, 2022
Fluent APIs aren't the norm in the System namespace anyway (though people do ask for them often.)
adamsitnik commentedon Nov 14, 2022
We have removed
CompletionSourceListin #1946 and we are now usingICollection<Func<CompletionContext, IEnumerable<CompletionItem>>>, so it's impossible to move these extensions methods anywhere. But they allow for using following C# syntax:But... I am not sure if this feature is discoverable as
Completionsis not a collection ofstringsand seeing this work is honestly surprising to me. Also,AcceptOnlyFromAmongseems 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
AcceptOnlyFromAmonginstead?KalleOlaviNiemitalo commentedon Nov 14, 2022
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. SharpLabI think I'd prefer removing the
paramsand requiring callers to explicitly create the arrayCompletions = { new[] { "mon", "tues", "wed", "thur", "fri", "sat", "sun" } }. SharpLabjonsequitur commentedon Nov 14, 2022
I agree.
adamsitnik commentedon Nov 16, 2022
This does not answer my question:
AcceptOnlyFromAmongsets the right completions but it also verifies that parsed values belong to the set of legal values.jonsequitur commentedon Nov 16, 2022
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.