From 29b72d0ea09fa2823919e0bde5ffdf733fece062 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 15 Dec 2022 16:10:27 +0100 Subject: [PATCH 1/8] Argument.AcceptOnlyFromAmong should return void --- ...ommandLine_api_is_not_changed.approved.txt | 2 +- src/System.CommandLine.Tests/ArgumentTests.cs | 6 ++--- .../CompletionTests.cs | 19 ++++++++----- .../ParsingValidationTests.cs | 27 ++++++++++++------- src/System.CommandLine/Argument{T}.cs | 5 +--- 5 files changed, 36 insertions(+), 23 deletions(-) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index b43629d19f..a67eb005b5 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -23,7 +23,7 @@ System.CommandLine public System.Type ValueType { get; } public Argument AcceptLegalFileNamesOnly() public Argument AcceptLegalFilePathsOnly() - public Argument AcceptOnlyFromAmong(System.String[] values) + public System.Void AcceptOnlyFromAmong(System.String[] values) public System.Void SetDefaultValue(T value) public System.Void SetDefaultValueFactory(Func defaultValueFactory) public System.Void SetDefaultValueFactory(Func defaultValueFactory) diff --git a/src/System.CommandLine.Tests/ArgumentTests.cs b/src/System.CommandLine.Tests/ArgumentTests.cs index 72717c61e5..31baeef27e 100644 --- a/src/System.CommandLine.Tests/ArgumentTests.cs +++ b/src/System.CommandLine.Tests/ArgumentTests.cs @@ -769,8 +769,9 @@ public void OnlyTake_can_pass_on_all_tokens_from_a_single_arity_argument_to_anot [Fact] public void Argument_of_enum_can_limit_enum_members_as_valid_values() { - var argument = new Argument() - .AcceptOnlyFromAmong(ConsoleColor.Red.ToString(), ConsoleColor.Green.ToString()); + var argument = new Argument(); + argument.AcceptOnlyFromAmong(ConsoleColor.Red.ToString(), ConsoleColor.Green.ToString()); + Command command = new("set-color") { argument @@ -788,7 +789,6 @@ public void Argument_of_enum_can_limit_enum_members_as_valid_values() public void Argument_of_T_fluent_APIs_return_Argument_of_T() { Argument argument = new Argument("--path") - .AcceptOnlyFromAmong("text") .AcceptLegalFileNamesOnly() .AcceptLegalFilePathsOnly(); diff --git a/src/System.CommandLine.Tests/CompletionTests.cs b/src/System.CommandLine.Tests/CompletionTests.cs index cad2213ceb..298a51258c 100644 --- a/src/System.CommandLine.Tests/CompletionTests.cs +++ b/src/System.CommandLine.Tests/CompletionTests.cs @@ -698,15 +698,15 @@ public void When_parsing_from_text_then_argument_completions_are_based_on_the_pr { new Command("one") { - new Argument().AcceptOnlyFromAmong("one-a", "one-b", "one-c") + CreateArgumentWithAcceptOnlyFromAmong("one-a", "one-b", "one-c") }, new Command("two") { - new Argument().AcceptOnlyFromAmong("two-a", "two-b", "two-c") + CreateArgumentWithAcceptOnlyFromAmong("two-a", "two-b", "two-c") }, new Command("three") { - new Argument().AcceptOnlyFromAmong("three-a", "three-b", "three-c") + CreateArgumentWithAcceptOnlyFromAmong("three-a", "three-b", "three-c") } }; @@ -725,15 +725,15 @@ public void When_parsing_from_array_then_argument_completions_are_based_on_the_p { new Command("one") { - new Argument().AcceptOnlyFromAmong("one-a", "one-b", "one-c") + CreateArgumentWithAcceptOnlyFromAmong("one-a", "one-b", "one-c") }, new Command("two") { - new Argument().AcceptOnlyFromAmong("two-a", "two-b", "two-c") + CreateArgumentWithAcceptOnlyFromAmong("two-a", "two-b", "two-c") }, new Command("three") { - new Argument().AcceptOnlyFromAmong("three-a", "three-b", "three-c") + CreateArgumentWithAcceptOnlyFromAmong("three-a", "three-b", "three-c") } }; @@ -968,5 +968,12 @@ public void When_option_completions_are_available_then_they_are_suggested_when_a .Be( $"Cannot parse argument 'SleepyDay' for option '--day' as expected type 'System.DayOfWeek'. Did you mean one of the following?{NewLine}Friday{NewLine}Monday{NewLine}Saturday{NewLine}Sunday{NewLine}Thursday{NewLine}Tuesday{NewLine}Wednesday"); } + + private Argument CreateArgumentWithAcceptOnlyFromAmong(params string[] values) + { + Argument argument = new(); + argument.AcceptOnlyFromAmong(values); + return argument; + } } } diff --git a/src/System.CommandLine.Tests/ParsingValidationTests.cs b/src/System.CommandLine.Tests/ParsingValidationTests.cs index 4c4e7b2d57..e615af7f55 100644 --- a/src/System.CommandLine.Tests/ParsingValidationTests.cs +++ b/src/System.CommandLine.Tests/ParsingValidationTests.cs @@ -71,12 +71,14 @@ public void When_FromAmong_is_used_then_the_OptionResult_ErrorMessage_is_set() [Fact] // https://github.com/dotnet/command-line-api/issues/1475 public void When_FromAmong_is_used_then_the_ArgumentResult_ErrorMessage_is_set() { - var option = new Argument().AcceptOnlyFromAmong("a", "b"); - var command = new Command("test") { option }; + var argument = new Argument(); + argument.AcceptOnlyFromAmong("a", "b"); + + var command = new Command("test") { argument }; var parseResult = command.Parse("test c"); - parseResult.FindResultFor(option) + parseResult.FindResultFor(argument) .ErrorMessage .Should() .Be(parseResult.Errors.Single().Message) @@ -90,8 +92,8 @@ public void When_FromAmong_is_used_for_multiple_arguments_and_valid_input_is_pro { var command = new Command("set") { - new Argument("key").AcceptOnlyFromAmong("key1", "key2"), - new Argument("value").AcceptOnlyFromAmong("value1", "value2") + CreateArgumentWithAcceptOnlyFromAmong(name: "key", "key1", "key2"), + CreateArgumentWithAcceptOnlyFromAmong(name : "value", "value1", "value2") }; var result = command.Parse("set key1 value1"); @@ -104,8 +106,8 @@ public void When_FromAmong_is_used_for_multiple_arguments_and_invalid_input_is_p { var command = new Command("set") { - new Argument("key").AcceptOnlyFromAmong("key1", "key2"), - new Argument("value").AcceptOnlyFromAmong("value1", "value2") + CreateArgumentWithAcceptOnlyFromAmong(name : "key", "key1", "key2"), + CreateArgumentWithAcceptOnlyFromAmong(name : "value", "value1", "value2") }; var result = command.Parse("set not-key1 value1"); @@ -152,8 +154,8 @@ public void When_FromAmong_is_used_for_multiple_arguments_and_invalid_input_is_p { var command = new Command("set") { - new Argument("key").AcceptOnlyFromAmong("key1", "key2"), - new Argument("value").AcceptOnlyFromAmong("value1", "value2") + CreateArgumentWithAcceptOnlyFromAmong(name : "key", "key1", "key2"), + CreateArgumentWithAcceptOnlyFromAmong(name : "value", "value1", "value2") }; var result = command.Parse("set key1 not-value1"); @@ -1233,5 +1235,12 @@ internal void When_there_is_an_arity_error_then_further_errors_are_not_reported( .Should() .Be("Required argument missing for option: '-o'."); } + + private Argument CreateArgumentWithAcceptOnlyFromAmong(string name, params string[] values) + { + Argument argument = new(name); + argument.AcceptOnlyFromAmong(values); + return argument; + } } } diff --git a/src/System.CommandLine/Argument{T}.cs b/src/System.CommandLine/Argument{T}.cs index 85467134f2..facce84d9e 100644 --- a/src/System.CommandLine/Argument{T}.cs +++ b/src/System.CommandLine/Argument{T}.cs @@ -174,8 +174,7 @@ public void SetDefaultValueFactory(Func defaultValueFactory) /// Configures the argument to accept only the specified values, and to suggest them as command line completions. /// /// The values that are allowed for the argument. - /// The configured argument. - public Argument AcceptOnlyFromAmong(params string[] values) + public void AcceptOnlyFromAmong(params string[] values) { if (values is not null && values.Length > 0) { @@ -185,8 +184,6 @@ public Argument AcceptOnlyFromAmong(params string[] values) CompletionSources.Add(values); } - return this; - void UnrecognizedArgumentError(ArgumentResult argumentResult) { for (var i = 0; i < argumentResult.Tokens.Count; i++) From 7f97ea9932f802081df2b10f3297871e02033a47 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 15 Dec 2022 16:28:33 +0100 Subject: [PATCH 2/8] Option.AcceptOnlyFromAmong should return void --- ...ommandLine_api_is_not_changed.approved.txt | 2 +- .../CompletionContextTests.cs | 10 ++++- .../CompletionTests.cs | 43 ++++++++++--------- src/System.CommandLine.Tests/OptionTests.cs | 5 +-- .../ParseDiagramTests.cs | 5 ++- src/System.CommandLine.Tests/ParserTests.cs | 4 +- .../ParsingValidationTests.cs | 11 ++--- src/System.CommandLine/Option{T}.cs | 8 +--- 8 files changed, 46 insertions(+), 42 deletions(-) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index a67eb005b5..d01d52eca5 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -200,7 +200,7 @@ System.CommandLine .ctor(System.String[] aliases, Func defaultValueFactory, System.String description = null) public Option AcceptLegalFileNamesOnly() public Option AcceptLegalFilePathsOnly() - public Option AcceptOnlyFromAmong(System.String[] values) + public System.Void AcceptOnlyFromAmong(System.String[] values) public System.Void SetDefaultValue(T value) public System.Void SetDefaultValueFactory(Func defaultValueFactory) public static class OptionValidation diff --git a/src/System.CommandLine.Tests/CompletionContextTests.cs b/src/System.CommandLine.Tests/CompletionContextTests.cs index 4090dfd430..554bfe1d73 100644 --- a/src/System.CommandLine.Tests/CompletionContextTests.cs +++ b/src/System.CommandLine.Tests/CompletionContextTests.cs @@ -109,10 +109,13 @@ public void When_position_is_unspecified_in_string_command_line_ending_with_a_sp [Fact] public void When_position_is_greater_than_input_length_in_a_string_command_line_then_it_returns_empty() { + Option option1 = new ("--option1"); + option1.AcceptOnlyFromAmong("apple", "banana", "cherry", "durian"); + var command = new Command("the-command") { new Argument(), - new Option("--option1").AcceptOnlyFromAmong("apple", "banana", "cherry", "durian"), + option1, new Option("--option2") }; @@ -176,9 +179,12 @@ public void When_position_is_unspecified_in_array_command_line_and_final_token_m [Fact] public void When_position_is_unspecified_in_array_command_line_and_final_token_matches_an_argument_then_it_returns_the_argument_value() { + Option option1 = new("--option1"); + option1.AcceptOnlyFromAmong("apple", "banana", "cherry", "durian"); + var command = new Command("the-command") { - new Option("--option1").AcceptOnlyFromAmong("apple", "banana", "cherry", "durian"), + option1, new Option("--option2"), new Argument() }; diff --git a/src/System.CommandLine.Tests/CompletionTests.cs b/src/System.CommandLine.Tests/CompletionTests.cs index 298a51258c..461a52aa37 100644 --- a/src/System.CommandLine.Tests/CompletionTests.cs +++ b/src/System.CommandLine.Tests/CompletionTests.cs @@ -441,8 +441,8 @@ public void Parser_options_can_supply_context_sensitive_matches() { var parser = new RootCommand { - new Option("--bread").AcceptOnlyFromAmong("wheat", "sourdough", "rye"), - new Option("--cheese").AcceptOnlyFromAmong("provolone", "cheddar", "cream cheese") + CreateOptionWithAcceptOnlyFromAmong(name: "--bread", "wheat", "sourdough", "rye"), + CreateOptionWithAcceptOnlyFromAmong(name: "--cheese", "provolone", "cheddar", "cream cheese") }; var commandLine = "--bread"; @@ -546,8 +546,8 @@ public void Argument_completions_can_be_based_on_the_proximate_option() var parser = new Parser( new Command("outer") { - new Option("--one").AcceptOnlyFromAmong("one-a", "one-b"), - new Option("--two").AcceptOnlyFromAmong("two-a", "two-b") + CreateOptionWithAcceptOnlyFromAmong(name: "--one", "one-a", "one-b"), + CreateOptionWithAcceptOnlyFromAmong(name: "--two", "two-a", "two-b") }); var commandLine = "outer --two"; @@ -648,12 +648,9 @@ public void When_caller_does_the_tokenizing_then_argument_completions_are_based_ { var command = new Command("outer") { - new Option("one") - .AcceptOnlyFromAmong("one-a", "one-b", "one-c"), - new Option("two") - .AcceptOnlyFromAmong("two-a", "two-b", "two-c"), - new Option("three") - .AcceptOnlyFromAmong("three-a", "three-b", "three-c") + CreateOptionWithAcceptOnlyFromAmong(name: "one", "one-a", "one-b", "one-c"), + CreateOptionWithAcceptOnlyFromAmong(name: "two", "two-a", "two-b", "two-c"), + CreateOptionWithAcceptOnlyFromAmong(name: "three", "three-a", "three-b", "three-c") }; var parser = new CommandLineBuilder(new RootCommand @@ -675,12 +672,9 @@ public void When_parsing_from_array_then_argument_completions_are_based_on_the_p { var command = new Command("outer") { - new Option("one") - .AcceptOnlyFromAmong("one-a", "one-b", "one-c"), - new Option("two") - .AcceptOnlyFromAmong("two-a", "two-b", "two-c"), - new Option("three") - .AcceptOnlyFromAmong("three-a", "three-b", "three-c") + CreateOptionWithAcceptOnlyFromAmong(name: "one", "one-a", "one-b", "one-c"), + CreateOptionWithAcceptOnlyFromAmong(name: "two", "two-a", "two-b", "two-c"), + CreateOptionWithAcceptOnlyFromAmong(name: "three", "three-a", "three-b", "three-c") }; var result = command.Parse("outer two b"); @@ -750,8 +744,8 @@ public void When_parsing_from_text_if_the_proximate_option_is_completed_then_com { var command = new RootCommand { - new Option("--framework").AcceptOnlyFromAmong("net7.0"), - new Option("--language").AcceptOnlyFromAmong("C#"), + CreateOptionWithAcceptOnlyFromAmong(name: "--framework", "net7.0"), + CreateOptionWithAcceptOnlyFromAmong(name: "--language", "C#"), new Option("--langVersion") }; var parser = new CommandLineBuilder(command).Build(); @@ -767,8 +761,8 @@ public void When_parsing_from_array_if_the_proximate_option_is_completed_then_co { var command = new RootCommand { - new Option("--framework").AcceptOnlyFromAmong("net7.0"), - new Option("--language").AcceptOnlyFromAmong("C#"), + CreateOptionWithAcceptOnlyFromAmong(name: "--framework", "net7.0"), + CreateOptionWithAcceptOnlyFromAmong(name: "--language", "C#"), new Option("--langVersion") }; var parser = new CommandLineBuilder(command).Build(); @@ -969,11 +963,18 @@ public void When_option_completions_are_available_then_they_are_suggested_when_a $"Cannot parse argument 'SleepyDay' for option '--day' as expected type 'System.DayOfWeek'. Did you mean one of the following?{NewLine}Friday{NewLine}Monday{NewLine}Saturday{NewLine}Sunday{NewLine}Thursday{NewLine}Tuesday{NewLine}Wednesday"); } - private Argument CreateArgumentWithAcceptOnlyFromAmong(params string[] values) + private static Argument CreateArgumentWithAcceptOnlyFromAmong(params string[] values) { Argument argument = new(); argument.AcceptOnlyFromAmong(values); return argument; } + + private static Option CreateOptionWithAcceptOnlyFromAmong(string name, params string[] values) + { + Option argument = new(name); + argument.AcceptOnlyFromAmong(values); + return argument; + } } } diff --git a/src/System.CommandLine.Tests/OptionTests.cs b/src/System.CommandLine.Tests/OptionTests.cs index 24bf89f39a..e065b05bc7 100644 --- a/src/System.CommandLine.Tests/OptionTests.cs +++ b/src/System.CommandLine.Tests/OptionTests.cs @@ -358,8 +358,8 @@ public void Option_of_boolean_defaults_to_false_when_not_specified() [Fact] public void Option_of_enum_can_limit_enum_members_as_valid_values() { - var option = new Option("--color") - .AcceptOnlyFromAmong(ConsoleColor.Red.ToString(), ConsoleColor.Green.ToString()); + Option option = new("--color"); + option.AcceptOnlyFromAmong(ConsoleColor.Red.ToString(), ConsoleColor.Green.ToString()); var result = option.Parse("--color Fuschia"); @@ -373,7 +373,6 @@ public void Option_of_enum_can_limit_enum_members_as_valid_values() public void Option_of_T_fluent_APIs_return_Option_of_T() { Option option = new Option("--path") - .AcceptOnlyFromAmong("text") .AcceptLegalFileNamesOnly() .AcceptLegalFilePathsOnly(); diff --git a/src/System.CommandLine.Tests/ParseDiagramTests.cs b/src/System.CommandLine.Tests/ParseDiagramTests.cs index c5812b27b0..baaf6f6c32 100644 --- a/src/System.CommandLine.Tests/ParseDiagramTests.cs +++ b/src/System.CommandLine.Tests/ParseDiagramTests.cs @@ -32,9 +32,12 @@ public void Parse_result_diagram_helps_explain_parse_operation() [Fact] public void Parse_result_diagram_displays_unmatched_tokens() { + Option option = new ("-x"); + option.AcceptOnlyFromAmong("arg1", "arg2", "arg3"); + var command = new Command("command") { - new Option("-x").AcceptOnlyFromAmong("arg1", "arg2", "arg3") + option }; var result = command.Parse("command -x ar"); diff --git a/src/System.CommandLine.Tests/ParserTests.cs b/src/System.CommandLine.Tests/ParserTests.cs index 40a965e8e5..17a4f02301 100644 --- a/src/System.CommandLine.Tests/ParserTests.cs +++ b/src/System.CommandLine.Tests/ParserTests.cs @@ -350,8 +350,8 @@ public void Parser_root_Options_can_be_specified_multiple_times_and_their_argume [Fact] public void Options_can_be_specified_multiple_times_and_their_arguments_are_collated() { - var animalsOption = new Option(new[] { "-a", "--animals" }) - .AcceptOnlyFromAmong("dog", "cat", "sheep"); + var animalsOption = new Option(new[] { "-a", "--animals" }); + animalsOption.AcceptOnlyFromAmong("dog", "cat", "sheep"); var vegetablesOption = new Option(new[] { "-v", "--vegetables" }); var parser = new Parser( new Command("the-command") { diff --git a/src/System.CommandLine.Tests/ParsingValidationTests.cs b/src/System.CommandLine.Tests/ParsingValidationTests.cs index e615af7f55..ce6983955e 100644 --- a/src/System.CommandLine.Tests/ParsingValidationTests.cs +++ b/src/System.CommandLine.Tests/ParsingValidationTests.cs @@ -24,8 +24,8 @@ public ParsingValidationTests(ITestOutputHelper output) [Fact] public void When_an_option_accepts_only_specific_arguments_but_a_wrong_one_is_supplied_then_an_informative_error_is_returned() { - var option = new Option("-x") - .AcceptOnlyFromAmong("this", "that", "the-other-thing"); + var option = new Option("-x"); + option.AcceptOnlyFromAmong("this", "that", "the-other-thing"); var result = option.Parse("-x none-of-those"); @@ -40,8 +40,8 @@ public void When_an_option_accepts_only_specific_arguments_but_a_wrong_one_is_su [Fact] public void When_an_option_has_en_error_then_the_error_has_a_reference_to_the_option() { - var option = new Option("-x") - .AcceptOnlyFromAmong("this", "that"); + var option = new Option("-x"); + option.AcceptOnlyFromAmong("this", "that"); var result = option.Parse("-x something_else"); @@ -54,7 +54,8 @@ public void When_an_option_has_en_error_then_the_error_has_a_reference_to_the_op [Fact] // https://github.com/dotnet/command-line-api/issues/1475 public void When_FromAmong_is_used_then_the_OptionResult_ErrorMessage_is_set() { - var option = new Option("--opt").AcceptOnlyFromAmong("a", "b"); + var option = new Option("--opt"); + option.AcceptOnlyFromAmong("a", "b"); var command = new Command("test") { option }; var parseResult = command.Parse("test --opt c"); diff --git a/src/System.CommandLine/Option{T}.cs b/src/System.CommandLine/Option{T}.cs index 1f75694a78..0cc690340e 100644 --- a/src/System.CommandLine/Option{T}.cs +++ b/src/System.CommandLine/Option{T}.cs @@ -103,13 +103,7 @@ public void SetDefaultValueFactory(Func defaultValueFactory) => /// Configures the option to accept only the specified values, and to suggest them as command line completions. /// /// The values that are allowed for the option. - /// The configured option. - public Option AcceptOnlyFromAmong(params string[] values) - { - _argument.AcceptOnlyFromAmong(values); - - return this; - } + public void AcceptOnlyFromAmong(params string[] values) => _argument.AcceptOnlyFromAmong(values); /// /// Configures the option to accept only values representing legal file paths. From d3a8c222bfe2c2ee0458237578ca6716176aac14 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 15 Dec 2022 16:36:39 +0100 Subject: [PATCH 3/8] Argument.AcceptLegalFilePathsOnly should return void --- ...sts.System_CommandLine_api_is_not_changed.approved.txt | 2 +- src/System.CommandLine.Tests/ArgumentTests.cs | 3 +-- src/System.CommandLine.Tests/ParsingValidationTests.cs | 8 ++++++-- src/System.CommandLine/Argument{T}.cs | 4 +--- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index d01d52eca5..8d33fb7e4f 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -22,7 +22,7 @@ System.CommandLine public System.Boolean HasDefaultValue { get; } public System.Type ValueType { get; } public Argument AcceptLegalFileNamesOnly() - public Argument AcceptLegalFilePathsOnly() + public System.Void AcceptLegalFilePathsOnly() public System.Void AcceptOnlyFromAmong(System.String[] values) public System.Void SetDefaultValue(T value) public System.Void SetDefaultValueFactory(Func defaultValueFactory) diff --git a/src/System.CommandLine.Tests/ArgumentTests.cs b/src/System.CommandLine.Tests/ArgumentTests.cs index 31baeef27e..44e45b86fb 100644 --- a/src/System.CommandLine.Tests/ArgumentTests.cs +++ b/src/System.CommandLine.Tests/ArgumentTests.cs @@ -789,8 +789,7 @@ public void Argument_of_enum_can_limit_enum_members_as_valid_values() public void Argument_of_T_fluent_APIs_return_Argument_of_T() { Argument argument = new Argument("--path") - .AcceptLegalFileNamesOnly() - .AcceptLegalFilePathsOnly(); + .AcceptLegalFileNamesOnly(); argument.Should().BeOfType>(); } diff --git a/src/System.CommandLine.Tests/ParsingValidationTests.cs b/src/System.CommandLine.Tests/ParsingValidationTests.cs index ce6983955e..4218dd1293 100644 --- a/src/System.CommandLine.Tests/ParsingValidationTests.cs +++ b/src/System.CommandLine.Tests/ParsingValidationTests.cs @@ -560,9 +560,11 @@ public class PathValidity [Fact] public void LegalFilePathsOnly_rejects_command_arguments_containing_invalid_path_characters() { + Argument argument = new(); + argument.AcceptLegalFilePathsOnly(); var command = new Command("the-command") { - new Argument().AcceptLegalFilePathsOnly() + argument }; var invalidCharacter = Path.GetInvalidPathChars().First(c => c != '"'); @@ -600,9 +602,11 @@ public void LegalFilePathsOnly_rejects_option_arguments_containing_invalid_path_ [Fact] public void LegalFilePathsOnly_accepts_command_arguments_containing_valid_path_characters() { + Argument argument = new (); + argument.AcceptLegalFilePathsOnly(); var command = new Command("the-command") { - new Argument().AcceptLegalFilePathsOnly() + argument }; var validPathName = Directory.GetCurrentDirectory(); diff --git a/src/System.CommandLine/Argument{T}.cs b/src/System.CommandLine/Argument{T}.cs index facce84d9e..052d1953d3 100644 --- a/src/System.CommandLine/Argument{T}.cs +++ b/src/System.CommandLine/Argument{T}.cs @@ -205,7 +205,7 @@ void UnrecognizedArgumentError(ArgumentResult argumentResult) /// Configures the argument to accept only values representing legal file paths. /// /// The configured argument. - public Argument AcceptLegalFilePathsOnly() + public void AcceptLegalFilePathsOnly() { var invalidPathChars = Path.GetInvalidPathChars(); @@ -225,8 +225,6 @@ public Argument AcceptLegalFilePathsOnly() } } }); - - return this; } /// From 692cd59c5e4c8978c8d6f15b44ad466ff0056503 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 15 Dec 2022 16:38:46 +0100 Subject: [PATCH 4/8] Option.AcceptLegalFilePathsOnly should return void --- ...ts.System_CommandLine_api_is_not_changed.approved.txt | 2 +- src/System.CommandLine.Tests/OptionTests.cs | 3 +-- src/System.CommandLine.Tests/ParsingValidationTests.cs | 9 +++++++-- src/System.CommandLine/Option{T}.cs | 6 +----- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index 8d33fb7e4f..9d237c3512 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -199,7 +199,7 @@ System.CommandLine .ctor(System.String name, Func defaultValueFactory, System.String description = null) .ctor(System.String[] aliases, Func defaultValueFactory, System.String description = null) public Option AcceptLegalFileNamesOnly() - public Option AcceptLegalFilePathsOnly() + public System.Void AcceptLegalFilePathsOnly() public System.Void AcceptOnlyFromAmong(System.String[] values) public System.Void SetDefaultValue(T value) public System.Void SetDefaultValueFactory(Func defaultValueFactory) diff --git a/src/System.CommandLine.Tests/OptionTests.cs b/src/System.CommandLine.Tests/OptionTests.cs index e065b05bc7..19fd46d15f 100644 --- a/src/System.CommandLine.Tests/OptionTests.cs +++ b/src/System.CommandLine.Tests/OptionTests.cs @@ -373,8 +373,7 @@ public void Option_of_enum_can_limit_enum_members_as_valid_values() public void Option_of_T_fluent_APIs_return_Option_of_T() { Option option = new Option("--path") - .AcceptLegalFileNamesOnly() - .AcceptLegalFilePathsOnly(); + .AcceptLegalFileNamesOnly(); option.Should().BeOfType>(); } diff --git a/src/System.CommandLine.Tests/ParsingValidationTests.cs b/src/System.CommandLine.Tests/ParsingValidationTests.cs index 4218dd1293..28f78e301c 100644 --- a/src/System.CommandLine.Tests/ParsingValidationTests.cs +++ b/src/System.CommandLine.Tests/ParsingValidationTests.cs @@ -582,9 +582,11 @@ public void LegalFilePathsOnly_rejects_command_arguments_containing_invalid_path [Fact] public void LegalFilePathsOnly_rejects_option_arguments_containing_invalid_path_characters() { + Option option = new ("-x"); + option.AcceptLegalFilePathsOnly(); var command = new Command("the-command") { - new Option("-x").AcceptLegalFilePathsOnly() + option }; var invalidCharacter = Path.GetInvalidPathChars().First(c => c != '"'); @@ -620,9 +622,12 @@ public void LegalFilePathsOnly_accepts_command_arguments_containing_valid_path_c [Fact] public void LegalFilePathsOnly_accepts_option_arguments_containing_valid_path_characters() { + Option option = new ("-x"); + option.AcceptLegalFilePathsOnly(); + var command = new Command("the-command") { - new Option("-x").AcceptLegalFilePathsOnly() + option }; var validPathName = Directory.GetCurrentDirectory(); diff --git a/src/System.CommandLine/Option{T}.cs b/src/System.CommandLine/Option{T}.cs index 0cc690340e..5f1562fca7 100644 --- a/src/System.CommandLine/Option{T}.cs +++ b/src/System.CommandLine/Option{T}.cs @@ -109,11 +109,7 @@ public void SetDefaultValueFactory(Func defaultValueFactory) => /// Configures the option to accept only values representing legal file paths. /// /// The configured option. - public Option AcceptLegalFilePathsOnly() - { - _argument.AcceptLegalFilePathsOnly(); - return this; - } + public void AcceptLegalFilePathsOnly() => _argument.AcceptLegalFilePathsOnly(); /// /// Configures the option to accept only values representing legal file names. From 8768c6e05ea330ea865602f0958fcd108e3cff28 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 15 Dec 2022 16:40:53 +0100 Subject: [PATCH 5/8] Argument.AcceptLegalFileNamesOnly should return void --- ....System_CommandLine_api_is_not_changed.approved.txt | 2 +- src/System.CommandLine.Tests/ArgumentTests.cs | 4 ++-- src/System.CommandLine.Tests/ParsingValidationTests.cs | 10 ++++++++-- .../TestApps/Trimming/Program.cs | 3 ++- src/System.CommandLine/Argument{T}.cs | 6 +----- 5 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index 9d237c3512..3dcbfe9580 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -21,7 +21,7 @@ System.CommandLine .ctor(Func parse, System.Boolean isDefault = False) public System.Boolean HasDefaultValue { get; } public System.Type ValueType { get; } - public Argument AcceptLegalFileNamesOnly() + public System.Void AcceptLegalFileNamesOnly() public System.Void AcceptLegalFilePathsOnly() public System.Void AcceptOnlyFromAmong(System.String[] values) public System.Void SetDefaultValue(T value) diff --git a/src/System.CommandLine.Tests/ArgumentTests.cs b/src/System.CommandLine.Tests/ArgumentTests.cs index 44e45b86fb..94c2b2c1e3 100644 --- a/src/System.CommandLine.Tests/ArgumentTests.cs +++ b/src/System.CommandLine.Tests/ArgumentTests.cs @@ -788,8 +788,8 @@ public void Argument_of_enum_can_limit_enum_members_as_valid_values() [Fact] public void Argument_of_T_fluent_APIs_return_Argument_of_T() { - Argument argument = new Argument("--path") - .AcceptLegalFileNamesOnly(); + Argument argument = new Argument("--path"); + argument.AcceptLegalFileNamesOnly(); argument.Should().BeOfType>(); } diff --git a/src/System.CommandLine.Tests/ParsingValidationTests.cs b/src/System.CommandLine.Tests/ParsingValidationTests.cs index 28f78e301c..d840fb481e 100644 --- a/src/System.CommandLine.Tests/ParsingValidationTests.cs +++ b/src/System.CommandLine.Tests/ParsingValidationTests.cs @@ -644,9 +644,12 @@ public class FileNameValidity [Fact] public void LegalFileNamesOnly_rejects_command_arguments_containing_invalid_file_name_characters() { + Argument argument = new(); + argument.AcceptLegalFileNamesOnly(); + var command = new Command("the-command") { - new Argument().AcceptLegalFileNamesOnly() + argument }; var invalidCharacter = Path.GetInvalidFileNameChars().First(c => c != '"'); @@ -684,9 +687,12 @@ public void LegalFileNamesOnly_rejects_option_arguments_containing_invalid_file_ [Fact] public void LegalFileNamesOnly_accepts_command_arguments_containing_valid_file_name_characters() { + Argument argument = new (); + argument.AcceptLegalFileNamesOnly(); + var command = new Command("the-command") { - new Argument().AcceptLegalFileNamesOnly() + argument }; var validFileName = Path.GetFileName(Directory.GetCurrentDirectory()); diff --git a/src/System.CommandLine.Tests/TestApps/Trimming/Program.cs b/src/System.CommandLine.Tests/TestApps/Trimming/Program.cs index a41990eaaa..9fb532bf2b 100644 --- a/src/System.CommandLine.Tests/TestApps/Trimming/Program.cs +++ b/src/System.CommandLine.Tests/TestApps/Trimming/Program.cs @@ -1,7 +1,8 @@ using System.CommandLine; using System.CommandLine.Invocation; -var fileArgument = new Argument().AcceptLegalFileNamesOnly(); +var fileArgument = new Argument(); +fileArgument.AcceptLegalFileNamesOnly(); var command = new RootCommand { diff --git a/src/System.CommandLine/Argument{T}.cs b/src/System.CommandLine/Argument{T}.cs index 052d1953d3..554635e139 100644 --- a/src/System.CommandLine/Argument{T}.cs +++ b/src/System.CommandLine/Argument{T}.cs @@ -204,7 +204,6 @@ void UnrecognizedArgumentError(ArgumentResult argumentResult) /// /// Configures the argument to accept only values representing legal file paths. /// - /// The configured argument. public void AcceptLegalFilePathsOnly() { var invalidPathChars = Path.GetInvalidPathChars(); @@ -231,8 +230,7 @@ public void AcceptLegalFilePathsOnly() /// Configures the argument to accept only values representing legal file names. /// /// A parse error will result, for example, if file path separators are found in the parsed value. - /// The configured argument. - public Argument AcceptLegalFileNamesOnly() + public void AcceptLegalFileNamesOnly() { var invalidFileNameChars = Path.GetInvalidFileNameChars(); @@ -249,8 +247,6 @@ public Argument AcceptLegalFileNamesOnly() } } }); - - return this; } } } From 7ab0cf53194fb94842f10f2ff41bfea8f4e4150e Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 15 Dec 2022 16:46:58 +0100 Subject: [PATCH 6/8] Option.AcceptLegalFileNamesOnly should return void --- ....System_CommandLine_api_is_not_changed.approved.txt | 2 +- src/System.CommandLine.Tests/OptionTests.cs | 4 ++-- src/System.CommandLine.Tests/ParsingValidationTests.cs | 10 ++++++++-- src/System.CommandLine/Option{T}.cs | 8 +------- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index 3dcbfe9580..fb0f9affe1 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -198,7 +198,7 @@ System.CommandLine .ctor(System.String[] aliases, Func parseArgument, System.Boolean isDefault = False, System.String description = null) .ctor(System.String name, Func defaultValueFactory, System.String description = null) .ctor(System.String[] aliases, Func defaultValueFactory, System.String description = null) - public Option AcceptLegalFileNamesOnly() + public System.Void AcceptLegalFileNamesOnly() public System.Void AcceptLegalFilePathsOnly() public System.Void AcceptOnlyFromAmong(System.String[] values) public System.Void SetDefaultValue(T value) diff --git a/src/System.CommandLine.Tests/OptionTests.cs b/src/System.CommandLine.Tests/OptionTests.cs index 19fd46d15f..d423ac25fa 100644 --- a/src/System.CommandLine.Tests/OptionTests.cs +++ b/src/System.CommandLine.Tests/OptionTests.cs @@ -372,8 +372,8 @@ public void Option_of_enum_can_limit_enum_members_as_valid_values() [Fact] public void Option_of_T_fluent_APIs_return_Option_of_T() { - Option option = new Option("--path") - .AcceptLegalFileNamesOnly(); + Option option = new Option("--path"); + option.AcceptLegalFileNamesOnly(); option.Should().BeOfType>(); } diff --git a/src/System.CommandLine.Tests/ParsingValidationTests.cs b/src/System.CommandLine.Tests/ParsingValidationTests.cs index d840fb481e..913fcb057f 100644 --- a/src/System.CommandLine.Tests/ParsingValidationTests.cs +++ b/src/System.CommandLine.Tests/ParsingValidationTests.cs @@ -667,9 +667,12 @@ public void LegalFileNamesOnly_rejects_command_arguments_containing_invalid_file [Fact] public void LegalFileNamesOnly_rejects_option_arguments_containing_invalid_file_name_characters() { + Option option = new("-x"); + option.AcceptLegalFileNamesOnly(); + var command = new Command("the-command") { - new Option("-x").AcceptLegalFileNamesOnly() + option }; var invalidCharacter = Path.GetInvalidFileNameChars().First(c => c != '"'); @@ -706,9 +709,12 @@ public void LegalFileNamesOnly_accepts_command_arguments_containing_valid_file_n [Fact] public void LegalFileNamesOnly_accepts_option_arguments_containing_valid_file_name_characters() { + Option option = new("-x"); + option.AcceptLegalFileNamesOnly(); + var command = new Command("the-command") { - new Option("-x").AcceptLegalFileNamesOnly() + option }; var validFileName = Path.GetFileName(Directory.GetCurrentDirectory()); diff --git a/src/System.CommandLine/Option{T}.cs b/src/System.CommandLine/Option{T}.cs index 5f1562fca7..7b6660ded7 100644 --- a/src/System.CommandLine/Option{T}.cs +++ b/src/System.CommandLine/Option{T}.cs @@ -108,18 +108,12 @@ public void SetDefaultValueFactory(Func defaultValueFactory) => /// /// Configures the option to accept only values representing legal file paths. /// - /// The configured option. public void AcceptLegalFilePathsOnly() => _argument.AcceptLegalFilePathsOnly(); /// /// Configures the option to accept only values representing legal file names. /// /// A parse error will result, for example, if file path separators are found in the parsed value. - /// The configured option. - public Option AcceptLegalFileNamesOnly() - { - _argument.AcceptLegalFileNamesOnly(); - return this; - } + public void AcceptLegalFileNamesOnly() => _argument.AcceptLegalFileNamesOnly(); } } \ No newline at end of file From f2d5d2fbef7570f9822255b22527e3f889cb6576 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 15 Dec 2022 17:31:20 +0100 Subject: [PATCH 7/8] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: David Cantú --- src/System.CommandLine.Tests/CompletionTests.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/System.CommandLine.Tests/CompletionTests.cs b/src/System.CommandLine.Tests/CompletionTests.cs index 461a52aa37..ab238d6b68 100644 --- a/src/System.CommandLine.Tests/CompletionTests.cs +++ b/src/System.CommandLine.Tests/CompletionTests.cs @@ -972,9 +972,9 @@ private static Argument CreateArgumentWithAcceptOnlyFromAmong(params str private static Option CreateOptionWithAcceptOnlyFromAmong(string name, params string[] values) { - Option argument = new(name); - argument.AcceptOnlyFromAmong(values); - return argument; + Option option = new(name); + option.AcceptOnlyFromAmong(values); + return option; } } } From 5b0012d759f6a8afef3ce601180ed3878f99dfbb Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 15 Dec 2022 17:43:00 +0100 Subject: [PATCH 8/8] address code review feedback: remove test that makes no sense now --- src/System.CommandLine.Tests/ArgumentTests.cs | 9 --------- src/System.CommandLine.Tests/OptionTests.cs | 9 --------- 2 files changed, 18 deletions(-) diff --git a/src/System.CommandLine.Tests/ArgumentTests.cs b/src/System.CommandLine.Tests/ArgumentTests.cs index 94c2b2c1e3..bd4db062ee 100644 --- a/src/System.CommandLine.Tests/ArgumentTests.cs +++ b/src/System.CommandLine.Tests/ArgumentTests.cs @@ -785,15 +785,6 @@ public void Argument_of_enum_can_limit_enum_members_as_valid_values() .BeEquivalentTo(new[] { $"Argument 'Fuschia' not recognized. Must be one of:\n\t'Red'\n\t'Green'" }); } - [Fact] - public void Argument_of_T_fluent_APIs_return_Argument_of_T() - { - Argument argument = new Argument("--path"); - argument.AcceptLegalFileNamesOnly(); - - argument.Should().BeOfType>(); - } - protected override Symbol CreateSymbol(string name) { return new Argument(name); diff --git a/src/System.CommandLine.Tests/OptionTests.cs b/src/System.CommandLine.Tests/OptionTests.cs index d423ac25fa..5716012e6d 100644 --- a/src/System.CommandLine.Tests/OptionTests.cs +++ b/src/System.CommandLine.Tests/OptionTests.cs @@ -368,15 +368,6 @@ public void Option_of_enum_can_limit_enum_members_as_valid_values() .Should() .BeEquivalentTo(new[] { $"Argument 'Fuschia' not recognized. Must be one of:\n\t'Red'\n\t'Green'" }); } - - [Fact] - public void Option_of_T_fluent_APIs_return_Option_of_T() - { - Option option = new Option("--path"); - option.AcceptLegalFileNamesOnly(); - - option.Should().BeOfType>(); - } protected override Symbol CreateSymbol(string name) => new Option(name); }