Skip to content

Expose List properties, remove Add* methods #1987

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Dec 15, 2022
Merged

Expose List properties, remove Add* methods #1987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
df9f188
expose Argument.Validators, remove AddValidator helper method
adamsitnik Dec 9, 2022
371cb1f
expose Option.Validators, remove AddValidator helper method
adamsitnik Dec 9, 2022
7946cb0
expose Command.Validators, remove AddValidator helper method
adamsitnik Dec 9, 2022
42746f1
Argument.Completions should return a List rather than ICollection, re…
adamsitnik Dec 9, 2022
ab6c6cb
expose Option.Completions, remove AddCompletions helper methods
adamsitnik Dec 9, 2022
c2a7102
address code review feedback: rename Completions to CompletionSources
adamsitnik Dec 15, 2022
9d461f4
Merge remote-tracking branch 'upstream/main' into collections
adamsitnik Dec 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Common/OptionBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public static Option CreateOption(string name, Type valueType, string descriptio
var optionType = typeof(Option<>).MakeGenericType(valueType);

#if NET6_0_OR_GREATER
var ctor = (ConstructorInfo)optionType.GetMemberWithSameMetadataDefinitionAs(_ctor);
var ctor = (ConstructorInfo)optionType.GetMemberWithSameMetadataDefinitionAs(_ctor);
#else
var ctor = optionType.GetConstructor(new[] { typeof(string), typeof(string) });
#endif
Expand Down
19 changes: 7 additions & 12 deletions ...ty.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
System.CommandLine
public abstract class Argument : Symbol, System.CommandLine.Binding.IValueDescriptor
public ArgumentArity Arity { get; set; }
public System.Collections.Generic.ICollection<System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem>>> Completions { get; }
public System.Collections.Generic.List<System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem>>> CompletionSources { get; }
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jozkee that is a very good point!

@KathleenDollard what is your opinion on this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, I've applied the rename and the PR is ready for review again. Thank you both for your feedback!

public System.Object GetDefaultValue()
Expand All @@ -23,10 +24,6 @@ System.CommandLine
public Argument<T> AcceptLegalFileNamesOnly()
public Argument<T> AcceptLegalFilePathsOnly()
public Argument<T> AcceptOnlyFromAmong(System.String[] values)
public Argument<T> AddCompletions(System.String[] completions)
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)
public Argument<T> AddValidator(System.Action<System.CommandLine.Parsing.ArgumentResult> validate)
public System.Void SetDefaultValue(T value)
public System.Void SetDefaultValueFactory(Func<T> defaultValueFactory)
public System.Void SetDefaultValueFactory(Func<System.CommandLine.Parsing.ArgumentResult,T> defaultValueFactory)
Expand Down Expand Up @@ -55,14 +52,14 @@ System.CommandLine
public System.Collections.Generic.IReadOnlyList<Option> Options { get; }
public System.Collections.Generic.IReadOnlyList<Command> Subcommands { get; }
public System.Boolean TreatUnmatchedTokensAsErrors { get; set; }
public System.Collections.Generic.List<System.Action<System.CommandLine.Parsing.CommandResult>> Validators { get; }
public System.Void Add(Option option)
public System.Void Add(Argument argument)
public System.Void Add(Command command)
public System.Void AddArgument(Argument argument)
public System.Void AddCommand(Command command)
public System.Void AddGlobalOption(Option option)
public System.Void AddOption(Option option)
public System.Void AddValidator(System.Action<System.CommandLine.Parsing.CommandResult> validate)
public System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem> GetCompletions(System.CommandLine.Completions.CompletionContext context)
public System.Collections.Generic.IEnumerator<Symbol> GetEnumerator()
public static class CommandExtensions
Expand Down Expand Up @@ -111,8 +108,8 @@ System.CommandLine
.ctor()
.ctor(System.String message, System.Exception innerException)
public static class CompletionSourceExtensions
public static System.Void Add(this System.Collections.Generic.ICollection<System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem>>> completionSources, System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.String>> completionsDelegate)
public static System.Void Add(this System.Collections.Generic.ICollection<System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem>>> completionSources, System.String[] completions)
public static System.Void Add(this System.Collections.Generic.List<System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem>>> completionSources, System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.String>> completionsDelegate)
public static System.Void Add(this System.Collections.Generic.List<System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem>>> completionSources, System.String[] completions)
public static class ConsoleExtensions
public static System.Void Write(this IConsole console, System.String value)
public static System.Void WriteLine(this IConsole console, System.String value)
Expand Down Expand Up @@ -192,7 +189,9 @@ System.CommandLine
public System.Boolean AllowMultipleArgumentsPerToken { get; set; }
public System.String ArgumentHelpName { get; set; }
public ArgumentArity Arity { get; set; }
public System.Collections.Generic.List<System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem>>> CompletionSources { get; }
public System.Boolean IsRequired { get; set; }
public System.Collections.Generic.List<System.Action<System.CommandLine.Parsing.OptionResult>> Validators { get; }
public System.Type ValueType { get; }
public System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem> GetCompletions(System.CommandLine.Completions.CompletionContext context)
public ParseResult Parse(System.String commandLine)
Expand All @@ -207,10 +206,6 @@ System.CommandLine
public Option<T> AcceptLegalFileNamesOnly()
public Option<T> AcceptLegalFilePathsOnly()
public Option<T> AcceptOnlyFromAmong(System.String[] values)
public Option<T> AddCompletions(System.String[] completions)
public Option<T> AddCompletions(System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.String>> completionsDelegate)
public Option<T> AddCompletions(System.Func<System.CommandLine.Completions.CompletionContext,System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem>> completionsDelegate)
public Option<T> AddValidator(System.Action<System.CommandLine.Parsing.OptionResult> validate)
public System.Void SetDefaultValue(T value)
public System.Void SetDefaultValueFactory(Func<T> defaultValueFactory)
public static class OptionValidation
Expand Down
10 changes: 8 additions & 2 deletions src/System.CommandLine.Benchmarks/CommandLine/Perf_Parser_Directives_Suggest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,16 @@ public void Setup()
{
_nullConsole = new NullConsole();

Option<string> fruitOption = new("--fruit");
fruitOption.CompletionSources.Add("apple", "banana", "cherry");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This naming now looks wrong. The things being added here are really completion items, not completion sources.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonsequitur I've tried renaming the Add extension method to AddCompletionItems but it break the C# duck typing for collection initialization:

image

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


Option<string> vegetableOption = new("--vegetable");
vegetableOption.CompletionSources.Add("asparagus", "broccoli", "carrot");

var eatCommand = new Command("eat")
{
new Option<string>("--fruit").AddCompletions("apple", "banana", "cherry"),
new Option<string>("--vegetable").AddCompletions("asparagus", "broccoli", "carrot")
fruitOption,
vegetableOption
};

_testParser = new CommandLineBuilder(eatCommand)
Expand Down
6 changes: 3 additions & 3 deletions src/System.CommandLine.Benchmarks/CommandLine/Perf_Suggestions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace System.CommandLine.Benchmarks.CommandLine
[BenchmarkCategory(Categories.CommandLine)]
public class Perf_Suggestions
{
private Symbol _testSymbol;
private Option _testSymbol;
private ParseResult _testParseResult;

/// <remarks>
Expand All @@ -37,8 +37,8 @@ private IEnumerable<Option> GenerateOptionsArray(int count)
[GlobalSetup(Target = nameof(SuggestionsFromSymbol))]
public void Setup_FromSymbol()
{
_testSymbol = new Option<string>("--hello")
.AddCompletions(GenerateSuggestionsArray(TestSuggestionsCount));
_testSymbol = new Option<string>("--hello");
_testSymbol.CompletionSources.Add(GenerateSuggestionsArray(TestSuggestionsCount));
}

[Benchmark]
Expand Down
4 changes: 0 additions & 4 deletions src/System.CommandLine.Tests/ArgumentTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -789,10 +789,6 @@ public void Argument_of_T_fluent_APIs_return_Argument_of_T()
{
Argument<string> argument = new Argument<string>("--path")
.AcceptOnlyFromAmong("text")
.AddCompletions("test")
.AddCompletions(ctx => Array.Empty<string>())
.AddCompletions(ctx => Array.Empty<CompletionItem>())
.AddValidator(_ => { })
.AcceptLegalFileNamesOnly()
.AcceptLegalFilePathsOnly();

Expand Down
44 changes: 25 additions & 19 deletions src/System.CommandLine.Tests/CompletionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ public CompletionTests(ITestOutputHelper output)
[Fact]
public void Option_GetCompletions_returns_argument_completions_if_configured()
{
var option = new Option<string>("--hello")
.AddCompletions("one", "two", "three");
var option = new Option<string>("--hello");
option.CompletionSources.Add("one", "two", "three");

var completions = option.GetCompletions(CompletionContext.Empty);

Expand Down Expand Up @@ -122,7 +122,7 @@ public void Command_GetCompletions_returns_available_subcommands_and_option_alia
new Argument<string[]>
{
Arity = ArgumentArity.OneOrMore,
Completions = { "command-argument" }
CompletionSources = { "command-argument" }
}
};

Expand Down Expand Up @@ -211,17 +211,19 @@ public void When_an_option_has_a_default_value_it_will_still_be_suggested()
public void Command_GetCompletions_can_access_ParseResult()
{
var originOption = new Option<string>("--origin");
var cloneOption = new Option<string>("--clone");

cloneOption.CompletionSources.Add(ctx =>
{
var opt1Value = ctx.ParseResult.GetValue(originOption);
return !string.IsNullOrWhiteSpace(opt1Value) ? new[] { opt1Value } : Array.Empty<string>();
});

var parser = new Parser(
new RootCommand
{
originOption,
new Option<string>("--clone")
.AddCompletions(ctx =>
{
var opt1Value = ctx.ParseResult.GetValue(originOption);
return !string.IsNullOrWhiteSpace(opt1Value) ? new[] { opt1Value } : Array.Empty<string>();
})
cloneOption
});

var result = parser.Parse("--origin test --clone ");
Expand Down Expand Up @@ -579,10 +581,12 @@ public void Option_GetCompletions_can_be_based_on_the_proximate_option_and_parti
[Fact]
public void Completions_can_be_provided_in_the_absence_of_validation()
{
Option<string> option = new ("-t");
option.CompletionSources.Add("vegetable", "mineral", "animal");

var command = new Command("the-command")
{
new Option<string>("-t")
.AddCompletions("vegetable", "mineral", "animal")
option
};

command.Parse("the-command -t m")
Expand All @@ -607,7 +611,7 @@ public void Command_argument_completions_can_be_provided_using_a_delegate()
{
new Argument<string>
{
Completions = { _ => new[] { "vegetable", "mineral", "animal" } }
CompletionSources = { _ => new[] { "vegetable", "mineral", "animal" } }
}
}
};
Expand All @@ -622,10 +626,12 @@ public void Command_argument_completions_can_be_provided_using_a_delegate()
[Fact]
public void Option_argument_completions_can_be_provided_using_a_delegate()
{
var option = new Option<string>("-x");
option.CompletionSources.Add(_ => new[] { "vegetable", "mineral", "animal" });

var command = new Command("the-command")
{
new Option<string>("-x")
.AddCompletions(_ => new [] { "vegetable", "mineral", "animal" })
option
};

var parseResult = command.Parse("the-command -x m");
Expand Down Expand Up @@ -851,8 +857,8 @@ public void It_can_provide_completions_within_quotes(string commandLine, int pos
"\"nuget:Microsoft.DotNet.Interactive\""
};

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

var r = new Command("#r")
{
Expand All @@ -873,8 +879,8 @@ public void It_can_provide_completions_within_quotes(string commandLine, int pos
public void Default_completions_can_be_cleared_and_replaced()
{
var argument = new Argument<DayOfWeek>();
argument.Completions.Clear();
argument.Completions.Add(new[] { "mon", "tues", "wed", "thur", "fri", "sat", "sun" });
argument.CompletionSources.Clear();
argument.CompletionSources.Add(new[] { "mon", "tues", "wed", "thur", "fri", "sat", "sun" });
var command = new Command("the-command")
{
argument
Expand All @@ -895,7 +901,7 @@ public void Default_completions_can_be_appended_to()
{
new Argument<DayOfWeek>
{
Completions = { "mon", "tues", "wed", "thur", "fri", "sat", "sun" }
CompletionSources = { "mon", "tues", "wed", "thur", "fri", "sat", "sun" }
}
};

Expand Down
6 changes: 1 addition & 5 deletions src/System.CommandLine.Tests/OptionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ public void Option_T_default_value_factory_can_be_set_after_instantiation()
public void Option_T_default_value_is_validated()
{
var option = new Option<int>("-x", () => 123);
option.AddValidator(symbol =>
option.Validators.Add(symbol =>
symbol.ErrorMessage = symbol.Tokens
.Select(t => t.Value)
.Where(v => v == "123")
Expand Down Expand Up @@ -374,10 +374,6 @@ public void Option_of_T_fluent_APIs_return_Option_of_T()
{
Option<string> option = new Option<string>("--path")
.AcceptOnlyFromAmong("text")
.AddCompletions("test")
.AddCompletions(ctx => Array.Empty<string>())
.AddCompletions(ctx => Array.Empty<CompletionItem>())
.AddValidator(_ => { })
.AcceptLegalFileNamesOnly()
.AcceptLegalFilePathsOnly();

Expand Down
Loading