Expose List properties, remove Add* methods #1987
Conversation
…move Argument<T>.AddCompletions
| public Argument<T> AddCompletions(System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.String>> completionsDelegate) | ||
| public Argument<T> AddCompletions(System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem>> completionsDelegate) |
There was a problem hiding this comment.
?
| public Argument<T> AddCompletions(System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.String>> completionsDelegate) | |
| public Argument<T> AddCompletions(System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem>> completionsDelegate) |
There was a problem hiding this comment.
@jozkee great catch, #solved
| public System.Boolean HasDefaultValue { get; } | ||
| public System.String HelpName { get; set; } | ||
| public System.Collections.Generic.List<System.Action<System.CommandLine.Parsing.ArgumentResult>> Validators { get; } | ||
| public System.Type ValueType { get; } | ||
| public System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem> GetCompletions(System.CommandLine.Completions.CompletionContext context) |
There was a problem hiding this comment.
The difference between GetCompletions() and Completions is that the former is the result of evaluating the Funcs in the latter, right? Shouldn't we disambiguate? Maybe we can rename the List property to CompletionSources.
There was a problem hiding this comment.
@jozkee that is a very good point!
@KathleenDollard what is your opinion on this?
There was a problem hiding this comment.
I agree.
Can you search the code to ensure we do not already have something called "CompletionSources" as I vaguely remember that. We should update the List name.
There was a problem hiding this comment.
Awesome, I've applied the rename and the PR is ready for review again. Thank you both for your feedback!
|
I've addressed all the feedback so I am going to merge it to make some progress with other PRs. |
| @@ -22,10 +22,16 @@ public void Setup() | |||
| { | |||
| _nullConsole = new NullConsole(); | |||
|
|
|||
| Option<string> fruitOption = new("--fruit"); | |||
| fruitOption.CompletionSources.Add("apple", "banana", "cherry"); | |||
There was a problem hiding this comment.
This naming now looks wrong. The things being added here are really completion items, not completion sources.
There was a problem hiding this comment.
@jonsequitur I've tried renaming the Add extension method to AddCompletionItems but it break the C# duck typing for collection initialization:
Error CS1503 Argument 1: cannot convert from 'string' to 'System.Func<System.CommandLine.Completions.CompletionContext, System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem>>' System.CommandLine.Tests (net462), System.CommandLine.Tests (net7.0) D:\projects\command-line-api\src\System.CommandLine.Tests\CompletionTests.cs 125 Active
Based on feedback from @bartonjs
By exposing a single
List<T>returning property we allow the users to not only add new items, but also remove them (#1884)fixes #1972
fixes #1884