From 0a091b07d1f0cd545fdaf8b8c015f7c2567ad70a Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 28 Feb 2023 15:30:39 +0100 Subject: [PATCH 1/6] add the method so writing tests is possible --- src/System.CommandLine/ParseResult.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/System.CommandLine/ParseResult.cs b/src/System.CommandLine/ParseResult.cs index 76872fc967..4e47e9f093 100644 --- a/src/System.CommandLine/ParseResult.cs +++ b/src/System.CommandLine/ParseResult.cs @@ -129,6 +129,14 @@ CommandLineText is null public T? GetValue(Option option) => RootCommandResult.GetValue(option); + /// + /// Gets the parsed or default value for the specified symbol name, in the context of parsed command. + /// + /// The name of the Symbol for which to get a value. + /// The parsed value or a configured default. + public T? GetValue(string name) => default; + //=> CommandResult.GetValue(name); + /// public override string ToString() => $"{nameof(ParseResult)}: {this.Diagram()}"; From 8794b207a2eb485fe963c16baebea26d404ca4c9 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 28 Feb 2023 15:41:02 +0100 Subject: [PATCH 2/6] implement tests, mostly by extending existing GetValue(Symbol) tests --- .../Binding/TypeConversionTests.cs | 363 ++++-------------- .../GetValueByNameParserTests.cs | 133 +++++++ .../GetValueByNameTypeConversionTests.cs | 19 + .../ParserTests.DoubleDash.cs | 1 - src/System.CommandLine.Tests/ParserTests.cs | 56 +-- 5 files changed, 248 insertions(+), 324 deletions(-) create mode 100644 src/System.CommandLine.Tests/GetValueByNameParserTests.cs create mode 100644 src/System.CommandLine.Tests/GetValueByNameTypeConversionTests.cs diff --git a/src/System.CommandLine.Tests/Binding/TypeConversionTests.cs b/src/System.CommandLine.Tests/Binding/TypeConversionTests.cs index 8435b13bd8..73dfd09d47 100644 --- a/src/System.CommandLine.Tests/Binding/TypeConversionTests.cs +++ b/src/System.CommandLine.Tests/Binding/TypeConversionTests.cs @@ -14,15 +14,26 @@ namespace System.CommandLine.Tests.Binding { public class TypeConversionTests { + protected virtual T GetValue(Option option, string commandLine) + { + var result = new RootCommand { option }.Parse(commandLine); + return result.GetValue(option); + } + + protected virtual T GetValue(Argument argument, string commandLine) + { + var result = new RootCommand { argument }.Parse(commandLine); + return result.GetValue(argument); + } + [Fact] public void Option_argument_of_FileInfo_can_be_bound_without_custom_conversion_logic() { var option = new Option("--file"); var file = new FileInfo(Path.Combine(new DirectoryInfo("temp").FullName, "the-file.txt")); - var result = new RootCommand { option }.Parse($"--file {file.FullName}"); - result.GetValue(option) + GetValue(option, $"--file {file.FullName}") .Name .Should() .Be("the-file.txt"); @@ -88,12 +99,11 @@ public void Argument_of_array_of_FileInfo_can_be_called_without_custom_conversio var file1 = new FileInfo(Path.Combine(new DirectoryInfo("temp").FullName, "file1.txt")); var file2 = new FileInfo(Path.Combine(new DirectoryInfo("temp").FullName, "file2.txt")); - var result = new RootCommand { option }.Parse($"--file {file1.FullName} --file {file2.FullName}"); - result.GetValue(option) - .Select(fi => fi.Name) - .Should() - .BeEquivalentTo("file1.txt", "file2.txt"); + GetValue(option, $"--file {file1.FullName} --file {file2.FullName}") + .Select(fi => fi.Name) + .Should() + .BeEquivalentTo("file1.txt", "file2.txt"); } [Fact] @@ -231,9 +241,7 @@ public void Nullable_bool_parses_as_null_when_the_option_has_not_been_applied() { var option = new Option("-x"); - new RootCommand { option } - .Parse("") - .GetValue(option) + GetValue(option, "") .Should() .Be(null); } @@ -431,9 +439,8 @@ public void Values_can_be_correctly_converted_to_DateTime_without_the_parser_spe var option = new Option("-x"); var dateString = "2022-02-06T01:46:03.0000000-08:00"; - var value = new RootCommand { option }.Parse($"-x {dateString}").GetValue(option); - value.Should().Be(DateTime.Parse(dateString)); + GetValue(option, $"-x {dateString}").Should().Be(DateTime.Parse(dateString)); } @@ -443,9 +450,8 @@ public void Values_can_be_correctly_converted_to_nullable_DateTime_without_the_p var option = new Option("-x"); var dateString = "2022-02-06T01:46:03.0000000-08:00"; - var value = new RootCommand { option }.Parse($"-x {dateString}").GetValue(option); - value.Should().Be(DateTime.Parse(dateString)); + GetValue(option, $"-x {dateString}").Should().Be(DateTime.Parse(dateString)); } [Fact] @@ -454,9 +460,8 @@ public void Values_can_be_correctly_converted_to_DateTimeOffset_without_the_pars var option = new Option("-x"); var dateString = "2022-02-06T09:52:54.5275055-08:00"; - var value = new RootCommand { option }.Parse($"-x {dateString}").GetValue(option); - value.Should().Be(DateTime.Parse(dateString)); + GetValue(option, $"-x {dateString}").Should().Be(DateTime.Parse(dateString)); } [Fact] @@ -465,349 +470,164 @@ public void Values_can_be_correctly_converted_to_nullable_DateTimeOffset_without var option = new Option("-x"); var dateString = "2022-02-06T09:52:54.5275055-08:00"; - var value = new RootCommand { option }.Parse($"-x {dateString}").GetValue(option); - value.Should().Be(DateTime.Parse(dateString)); + GetValue(option, $"-x {dateString}").Should().Be(DateTime.Parse(dateString)); } [Fact] public void Values_can_be_correctly_converted_to_decimal_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var result = new RootCommand { option }.Parse("-x 123.456"); - - var value = result.GetValue(option); - - value.Should().Be(123.456m); - } + => GetValue(new Option("-x"), "-x 123.456").Should().Be(123.456m); [Fact] public void Values_can_be_correctly_converted_to_nullable_decimal_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x 123.456").GetValue(option); - - value.Should().Be(123.456m); - } + => GetValue(new Option("-x"), "-x 123.456").Should().Be(123.456m); [Fact] public void Values_can_be_correctly_converted_to_double_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x 123.456").GetValue(option); - - value.Should().Be(123.456d); - } + => GetValue(new Option("-x"), "-x 123.456").Should().Be(123.456d); [Fact] public void Values_can_be_correctly_converted_to_nullable_double_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x 123.456").GetValue(option); - - value.Should().Be(123.456d); - } + => GetValue(new Option("-x"), "-x 123.456").Should().Be(123.456d); [Fact] public void Values_can_be_correctly_converted_to_float_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x 123.456").GetValue(option); - - value.Should().Be(123.456f); - } + => GetValue(new Option("-x"), "-x 123.456").Should().Be(123.456f); [Fact] public void Values_can_be_correctly_converted_to_nullable_float_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x 123.456").GetValue(option); - - value.Should().Be(123.456f); - } + => GetValue(new Option("-x"), "-x 123.456").Should().Be(123.456f); [Fact] public void Values_can_be_correctly_converted_to_Guid_without_the_parser_specifying_a_custom_converter() { - var guidString = "75517282-018F-46BB-B15F-1D8DBFE23F6E"; - var option = new Option("-x"); + const string guidString = "75517282-018F-46BB-B15F-1D8DBFE23F6E"; - var value = new RootCommand { option }.Parse($"-x {guidString}").GetValue(option); - - value.Should().Be(Guid.Parse(guidString)); + GetValue(new Option("-x"), $"-x {guidString}").Should().Be(Guid.Parse(guidString)); } [Fact] public void Values_can_be_correctly_converted_to_nullable_Guid_without_the_parser_specifying_a_custom_converter() { - var guidString = "75517282-018F-46BB-B15F-1D8DBFE23F6E"; - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse($"-x {guidString}").GetValue(option); + const string guidString = "75517282-018F-46BB-B15F-1D8DBFE23F6E"; - value.Should().Be(Guid.Parse(guidString)); + GetValue(new Option("-x"), $"-x {guidString}").Should().Be(Guid.Parse(guidString)); } [Fact] public void Values_can_be_correctly_converted_to_TimeSpan_without_the_parser_specifying_a_custom_converter() { - var timeSpanString = "30"; - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse($"-x {timeSpanString}").GetValue(option); + const string timeSpanString = "30"; - value.Should().Be(TimeSpan.Parse(timeSpanString)); + GetValue(new Option("-x"), $"-x {timeSpanString}").Should().Be(TimeSpan.Parse(timeSpanString)); } [Fact] public void Values_can_be_correctly_converted_to_nullable_TimeSpan_without_the_parser_specifying_a_custom_converter() { - var timeSpanString = "30"; - var option = new Option("-x"); + const string timeSpanString = "30"; - var value = new RootCommand { option }.Parse($"-x {timeSpanString}").GetValue(option); - - value.Should().Be(TimeSpan.Parse(timeSpanString)); + GetValue(new Option("-x"), $"-x {timeSpanString}").Should().Be(TimeSpan.Parse(timeSpanString)); } [Fact] public void Values_can_be_correctly_converted_to_Uri_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x http://example.com").GetValue(option); - - value.Should().BeEquivalentTo(new Uri("http://example.com")); - } + => GetValue(new Option("-x"), "-x http://example.com").Should().BeEquivalentTo(new Uri("http://example.com")); [Fact] public void Options_with_arguments_specified_can_be_correctly_converted_to_bool_without_the_parser_specifying_a_custom_converter() { - var option = new Option("-x"); - - new RootCommand { option }.Parse("-x false").GetValue(option).Should().BeFalse(); - new RootCommand { option }.Parse("-x true").GetValue(option).Should().BeTrue(); + GetValue(new Option("-x"), "-x false").Should().BeFalse(); + GetValue(new Option("-x"), "-x true").Should().BeTrue(); } [Fact] public void Values_can_be_correctly_converted_to_long_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x 123456790").GetValue(option); - - value.Should().Be(123456790L); - } + => GetValue(new Option("-x"), "-x 123456790").Should().Be(123456790L); [Fact] public void Values_can_be_correctly_converted_to_nullable_long_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x 1234567890").GetValue(option); - - value.Should().Be(1234567890L); - } + => GetValue(new Option("-x"), "-x 123456790").Should().Be(123456790L); [Fact] public void Values_can_be_correctly_converted_to_short_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-s"); - - var value = new RootCommand { option }.Parse("-s 1234").GetValue(option); - - value.Should().Be(1234); - } + => GetValue(new Option("-s"), "-s 1234").Should().Be(1234); [Fact] public void Values_can_be_correctly_converted_to_nullable_short_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-s"); - - var value = new RootCommand { option }.Parse("-s 1234").GetValue(option); - - value.Should().Be(1234); - } + => GetValue(new Option("-s"), "-s 1234").Should().Be(1234); [Fact] public void Values_can_be_correctly_converted_to_ulong_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x 1234").GetValue(option); - - value.Should().Be(1234); - } + => GetValue(new Option("-x"), "-x 1234").Should().Be(1234); [Fact] public void Values_can_be_correctly_converted_to_nullable_ulong_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x 1234").GetValue(option); - - value.Should().Be(1234); - } + => GetValue(new Option("-x"), "-x 1234").Should().Be(1234); [Fact] public void Values_can_be_correctly_converted_to_ushort_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x 1234").GetValue(option); - - value.Should().Be(1234); - } + => GetValue(new Option("-s"), "-s 1234").Should().Be(1234); [Fact] public void Values_can_be_correctly_converted_to_nullable_ushort_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x 1234").GetValue(option); - - value.Should().Be(1234); - } + => GetValue(new Option("-s"), "-s 1234").Should().Be(1234); [Fact] public void Values_can_be_correctly_converted_to_sbyte_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-us"); - - var value = new RootCommand { option }.Parse("-us 123").GetValue(option); - - value.Should().Be(123); - } + => GetValue(new Option("-us"), "-us 123").Should().Be(123); [Fact] public void Values_can_be_correctly_converted_to_nullable_sbyte_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x 123").GetValue(option); - - value.Should().Be(123); - } + => GetValue(new Option("-us"), "-us 123").Should().Be(123); [Fact] public void Values_can_be_correctly_converted_to_ipaddress_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-us"); - - var value = new RootCommand { option }.Parse("-us 1.2.3.4").GetValue(option); - - value.Should().Be(IPAddress.Parse("1.2.3.4")); - } + => GetValue(new Option("-us"), "-us 1.2.3.4").Should().Be(IPAddress.Parse("1.2.3.4")); #if NETCOREAPP3_0_OR_GREATER [Fact] public void Values_can_be_correctly_converted_to_ipendpoint_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-us"); - - var value = new RootCommand { option }.Parse("-us 1.2.3.4:56").GetValue(option); - - value.Should().Be(IPEndPoint.Parse("1.2.3.4:56")); - } + => GetValue(new Option("-us"), "-us 1.2.3.4:56").Should().Be(IPEndPoint.Parse("1.2.3.4:56")); #endif #if NET6_0_OR_GREATER [Fact] public void Values_can_be_correctly_converted_to_dateonly_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-us"); - - var value = new RootCommand { option }.Parse("-us 2022-03-02").GetValue(option); - - value.Should().Be(DateOnly.Parse("2022-03-02")); - } + => GetValue(new Option("-us"), "-us 2022-03-02").Should().Be(DateOnly.Parse("2022-03-02")); [Fact] public void Values_can_be_correctly_converted_to_nullable_dateonly_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x 2022-03-02").GetValue(option); - - value.Should().Be(DateOnly.Parse("2022-03-02")); - } + => GetValue(new Option("-x"), "-x 2022-03-02").Should().Be(DateOnly.Parse("2022-03-02")); [Fact] public void Values_can_be_correctly_converted_to_timeonly_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-us"); - - var value = new RootCommand { option }.Parse("-us 12:34:56").GetValue(option); - - value.Should().Be(TimeOnly.Parse("12:34:56")); - } + => GetValue(new Option("-us"), "-us 12:34:56").Should().Be(TimeOnly.Parse("12:34:56")); [Fact] public void Values_can_be_correctly_converted_to_nullable_timeonly_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x 12:34:56").GetValue(option); - - value.Should().Be(TimeOnly.Parse("12:34:56")); - } + => GetValue(new Option("-x"), "-x 12:34:56").Should().Be(TimeOnly.Parse("12:34:56")); #endif [Fact] public void Values_can_be_correctly_converted_to_byte_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-us"); - - var value = new RootCommand { option }.Parse("-us 123").GetValue(option); - - value.Should().Be(123); - } + => GetValue(new Option("-us"), "-us 123").Should().Be(123); [Fact] public void Values_can_be_correctly_converted_to_nullable_byte_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x 123").GetValue(option); - - value.Should().Be(123); - } + => GetValue(new Option("-us"), "-us 123").Should().Be(123); [Fact] public void Values_can_be_correctly_converted_to_uint_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-us"); - - var value = new RootCommand { option }.Parse("-us 1234").GetValue(option); - - value.Should().Be(1234); - } + => GetValue(new Option("-us"), "-us 1234").Should().Be(1234); [Fact] public void Values_can_be_correctly_converted_to_nullable_uint_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x 1234").GetValue(option); - - value.Should().Be(1234); - } + => GetValue(new Option("-us"), "-us 1234").Should().Be(1234); [Fact] public void Values_can_be_correctly_converted_to_array_of_int_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var value = new RootCommand { option }.Parse("-x 1 -x 2 -x 3").GetValue(option); - - value.Should().BeEquivalentTo(1, 2, 3); - } + => GetValue(new Option("-x"), "-x 1 -x 2 -x 3").Should().BeEquivalentTo(1, 2, 3); [Theory] [InlineData(0, 100_000, typeof(string[]))] @@ -852,47 +672,19 @@ public void Max_arity_greater_than_1_converts_to_enumerable_types( [Fact] public void Values_can_be_correctly_converted_to_List_of_int_without_the_parser_specifying_a_custom_converter() - { - var option = new Option>("-x"); - - var value = new RootCommand { option }.Parse("-x 1 -x 2 -x 3").GetValue(option); - - value.Should().BeEquivalentTo(1, 2, 3); - } + => GetValue(new Option>("-x"), "-x 1 -x 2 -x 3").Should().BeEquivalentTo(1, 2, 3); [Fact] public void Values_can_be_correctly_converted_to_IEnumerable_of_int_without_the_parser_specifying_a_custom_converter() - { - var option = new Option>("-x"); - - var value = new RootCommand { option }.Parse("-x 1 -x 2 -x 3").GetValue(option); - - value.Should().BeEquivalentTo(1, 2, 3); - } + => GetValue(new Option>("-x"), "-x 1 -x 2 -x 3").Should().BeEquivalentTo(1, 2, 3); [Fact] public void Enum_values_can_be_correctly_converted_based_on_enum_value_name_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var parseResult = new RootCommand { option }.Parse("-x Monday"); - - var value = parseResult.GetValue(option); - - value.Should().Be(DayOfWeek.Monday); - } + => GetValue(new Option("-x"), "-x Monday").Should().Be(DayOfWeek.Monday); [Fact] public void Nullable_enum_values_can_be_correctly_converted_based_on_enum_value_name_without_the_parser_specifying_a_custom_converter() - { - var option = new Option("-x"); - - var parseResult = new RootCommand { option }.Parse("-x Monday"); - - var value = parseResult.GetValue(option); - - value.Should().Be(DayOfWeek.Monday); - } + => GetValue(new Option("-x"), "-x Monday").Should().Be(DayOfWeek.Monday); [Fact] public void Enum_values_that_cannot_be_parsed_result_in_an_informative_error() @@ -930,11 +722,7 @@ public void When_getting_a_single_value_and_specifying_a_conversion_type_that_is [Fact] public void When_getting_an_array_of_values_and_specifying_a_conversion_type_that_is_not_supported_then_it_throws() { - var option = new Option("-x"); - - var result = new RootCommand { option }.Parse("-x not-an-int -x 2"); - - Action getValue = () => result.GetValue(option); + Action getValue = () => GetValue(new Option("-x"), "-x not-an-int -x 2"); getValue.Should() .Throw() @@ -946,18 +734,7 @@ public void When_getting_an_array_of_values_and_specifying_a_conversion_type_tha [Fact] public void String_defaults_to_null_when_not_specified() - { - var argument = new Argument("arg"); - var command = new Command("mycommand") - { - argument - }; - - var result = command.Parse("mycommand"); - result.GetValue(argument) - .Should() - .BeNull(); - } + => GetValue(new Argument("arg"), "").Should().BeNull(); [Theory] [InlineData(typeof(List))] @@ -986,12 +763,6 @@ public void Sequence_type_defaults_to_empty_when_not_specified(Type sequenceType } private void AssertParsedValueIsEmpty(Argument argument) where T : IEnumerable - { - var result = new RootCommand { argument }.Parse(""); - - result.GetValue(argument) - .Should() - .BeEmpty(); - } + => GetValue(argument, "").Should().BeEmpty(); } } \ No newline at end of file diff --git a/src/System.CommandLine.Tests/GetValueByNameParserTests.cs b/src/System.CommandLine.Tests/GetValueByNameParserTests.cs new file mode 100644 index 0000000000..2e9647cb64 --- /dev/null +++ b/src/System.CommandLine.Tests/GetValueByNameParserTests.cs @@ -0,0 +1,133 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using FluentAssertions; +using System.Linq; +using Xunit; +using Xunit.Abstractions; + +namespace System.CommandLine.Tests +{ + public class GetValueByNameParserTests : ParserTests + { + public GetValueByNameParserTests(ITestOutputHelper output) : base(output) + { + } + + protected override T GetValue(ParseResult parseResult, Option option) + => parseResult.GetValue(option.Name); + + protected override T GetValue(ParseResult parseResult, Argument argument) + => parseResult.GetValue(argument.Name); + + [Fact] + public void In_case_of_argument_name_conflict_the_value_which_belongs_to_the_last_parsed_command_is_returned() + { + RootCommand command = new() + { + new Argument("arg"), + new Command("inner1") + { + new Argument("arg"), + new Command("inner2") + { + new Argument("arg"), + } + } + }; + + ParseResult parseResult = command.Parse("1 inner1 2 inner2 3"); + + parseResult.GetValue("arg").Should().Be(3); + } + + [Fact] + public void In_case_of_option_name_conflict_the_value_which_belongs_to_the_last_parsed_command_is_returned() + { + RootCommand command = new() + { + new Option("opt", new[] { "-i", "--integer" }), + new Command("inner1") + { + new Option("opt", new[] { "-i", "--integer" }), + new Command("inner2") + { + new Option("opt", new[] { "-i", "--integer" }) + } + } + }; + + ParseResult parseResult = command.Parse("-i 1 inner1 --integer 2 inner2 -i 3"); + + parseResult.GetValue("opt").Should().Be(3); + } + + [Fact] + public void When_option_value_is_not_parsed_then_default_value_is_returned() + { + RootCommand command = new() + { + new Option("opt", new[] { "-i", "--integer" }) + }; + + ParseResult parseResult = command.Parse(""); + + parseResult.GetValue("opt").Should().Be(default); + } + + [Fact] + public void When_argument_value_is_not_parsed_then_default_value_is_returned() + { + RootCommand command = new() + { + new Argument("arg") + }; + + ParseResult parseResult = command.Parse(""); + + parseResult.GetValue("arg").Should().Be(default); + } + + [Fact] + public void When_required_option_value_is_not_parsed_then_an_exception_is_thrown() + { + RootCommand command = new() + { + new Option("required", new[] { "-i", "--integer" }) + { + IsRequired = true + } + }; + + ParseResult parseResult = command.Parse(""); + + Action getRequired = () => parseResult.GetValue("required"); + + getRequired + .Should() + .Throw() + .Where(ex => ex.Message == LocalizationResources.RequiredOptionWasNotProvided("required")); + } + + [Fact] + public void When_required_argument_value_is_not_parsed_then_an_exception_is_thrown() + { + RootCommand command = new() + { + new Argument("required") + { + Arity = new ArgumentArity(1, 1) + } + }; + + ParseResult parseResult = command.Parse(""); + + Action getRequired = () => parseResult.GetValue("required"); + + getRequired + .Should() + .Throw() + .Where(ex => ex.Message == LocalizationResources.RequiredOptionWasNotProvided("required")); + } + } +} diff --git a/src/System.CommandLine.Tests/GetValueByNameTypeConversionTests.cs b/src/System.CommandLine.Tests/GetValueByNameTypeConversionTests.cs new file mode 100644 index 0000000000..11a6e81620 --- /dev/null +++ b/src/System.CommandLine.Tests/GetValueByNameTypeConversionTests.cs @@ -0,0 +1,19 @@ +using System.CommandLine.Tests.Binding; + +namespace System.CommandLine.Tests +{ + public class GetValueByNameTypeConversionTests : TypeConversionTests + { + protected override T GetValue(Argument argument, string commandLine) + { + var result = new RootCommand { argument }.Parse(commandLine); + return result.GetValue(argument.Name); + } + + protected override T GetValue(Option option, string commandLine) + { + var result = new RootCommand { option }.Parse(commandLine); + return result.GetValue(option.Name); + } + } +} diff --git a/src/System.CommandLine.Tests/ParserTests.DoubleDash.cs b/src/System.CommandLine.Tests/ParserTests.DoubleDash.cs index bf7572c455..7266d0c752 100644 --- a/src/System.CommandLine.Tests/ParserTests.DoubleDash.cs +++ b/src/System.CommandLine.Tests/ParserTests.DoubleDash.cs @@ -1,7 +1,6 @@ // Copyright (c) .NET Foundation and contributors. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. -using System.CommandLine.Parsing; using System.CommandLine.Tests.Utility; using FluentAssertions; using Xunit; diff --git a/src/System.CommandLine.Tests/ParserTests.cs b/src/System.CommandLine.Tests/ParserTests.cs index 824fb59afe..09cc5ddb52 100644 --- a/src/System.CommandLine.Tests/ParserTests.cs +++ b/src/System.CommandLine.Tests/ParserTests.cs @@ -23,6 +23,12 @@ public ParserTests(ITestOutputHelper output) _output = output; } + protected virtual T GetValue(ParseResult parseResult, Option option) + => parseResult.GetValue(option); + + protected virtual T GetValue(ParseResult parseResult, Argument argument) + => parseResult.GetValue(argument); + [Fact] public void An_option_can_be_checked_by_object_instance() { @@ -823,7 +829,7 @@ public void Commands_can_have_default_argument_values() ParseResult result = command.Parse("command"); - result.GetValue(argument) + GetValue(result, argument) .Should() .Be("default"); } @@ -841,7 +847,7 @@ public void When_an_option_with_a_default_value_is_not_matched_then_the_option_c ParseResult result = command.Parse("command"); result.FindResultFor(option).Should().NotBeNull(); - result.GetValue(option).Should().Be("the-default"); + GetValue(result, option).Should().Be("the-default"); } [Fact] @@ -921,8 +927,8 @@ public void Command_default_argument_value_does_not_override_parsed_value() var result = command.Parse("the-directory"); - result.GetValue(argument) - ?.Name + GetValue(result, argument) + .Name .Should() .Be("the-directory"); } @@ -1100,9 +1106,7 @@ public void Option_arguments_can_start_with_prefixes_that_make_them_look_like_op var result = command.Parse(input); - var valueForOption = result.GetValue(optionX); - - valueForOption.Should().Be("-y"); + GetValue(result, optionX).Should().Be("-y"); } [Fact] @@ -1121,9 +1125,9 @@ public void Option_arguments_can_start_with_prefixes_that_make_them_look_like_bu var result = command.Parse("-a -bc"); - result.GetValue(optionA).Should().Be("-bc"); - result.GetValue(optionB).Should().BeFalse(); - result.GetValue(optionC).Should().BeFalse(); + GetValue(result, optionA).Should().Be("-bc"); + GetValue(result, optionB).Should().BeFalse(); + GetValue(result, optionC).Should().BeFalse(); } [Fact] @@ -1140,7 +1144,7 @@ public void Option_arguments_can_match_subcommands() _output.WriteLine(result.ToString()); - result.GetValue(optionA).Should().Be("subcommand"); + GetValue(result, optionA).Should().Be("subcommand"); result.CommandResult.Command.Should().BeSameAs(root); } @@ -1161,7 +1165,7 @@ public void Arguments_can_match_subcommands() result.CommandResult.Command.Should().BeSameAs(subcommand); - result.GetValue(argument) + GetValue(result, argument) .Should() .BeEquivalentSequenceTo("one", "two", "three", "subcommand", "four"); } @@ -1181,10 +1185,8 @@ public void Option_arguments_can_match_the_aliases_of_sibling_options_when_non_s var result = command.Parse(input); - var valueForOption = result.GetValue(optionX); - result.Errors.Should().BeEmpty(); - valueForOption.Should().Be("-y"); + GetValue(result, optionX).Should().Be("-y"); } [Fact] @@ -1199,7 +1201,7 @@ public void Single_option_arguments_that_match_option_aliases_are_parsed_correct var result = command.Parse("-x -x"); - result.GetValue(optionX).Should().Be("-x"); + GetValue(result, optionX).Should().Be("-x"); } [Theory] @@ -1226,8 +1228,8 @@ public void Boolean_options_are_not_greedy(string commandLine) result.Errors.Should().BeEmpty(); - result.GetValue(optX).Should().BeTrue(); - result.GetValue(optY).Should().BeTrue(); + GetValue(result, optX).Should().BeTrue(); + GetValue(result, optY).Should().BeTrue(); } [Fact] @@ -1246,8 +1248,8 @@ public void Multiple_option_arguments_that_match_multiple_arity_option_aliases_a _output.WriteLine(result.Diagram()); - result.GetValue(optionX).Should().BeEquivalentTo(new[] { "-x", "-y", "-y" }); - result.GetValue(optionY).Should().BeEquivalentTo(new[] { "-x", "-y", "-x" }); + GetValue(result, optionX).Should().BeEquivalentTo(new[] { "-x", "-y", "-y" }); + GetValue(result, optionY).Should().BeEquivalentTo(new[] { "-x", "-y", "-x" }); } [Fact] @@ -1264,7 +1266,7 @@ public void Bundled_option_arguments_that_match_option_aliases_are_parsed_correc var result = command.Parse("-yxx"); - result.GetValue(optionX).Should().Be("x"); + GetValue(result, optionX).Should().Be("x"); } [Fact] @@ -1281,8 +1283,8 @@ public void Argument_name_is_not_matched_as_a_token() var result = command.Parse("name one two three"); - result.GetValue(nameArg).Should().Be("name"); - result.GetValue(columnsArg).Should().BeEquivalentTo("one", "two", "three"); + GetValue(result, nameArg).Should().Be("name"); + GetValue(result, columnsArg).Should().BeEquivalentTo("one", "two", "three"); } [Fact] @@ -1307,7 +1309,7 @@ public void Boolean_options_with_no_argument_specified_do_not_match_subsequent_a var result = command.Parse("-v an-argument"); - result.GetValue(option).Should().BeTrue(); + GetValue(result, option).Should().BeTrue(); } [Fact] @@ -1324,8 +1326,8 @@ public void When_a_command_line_has_unmatched_tokens_they_are_not_applied_to_sub var result = command.Parse("-x 23 unmatched-token -y 42"); - result.GetValue(optionX).Should().Be("23"); - result.GetValue(optionY).Should().Be("42"); + GetValue(result, optionX).Should().Be("23"); + GetValue(result, optionY).Should().Be("42"); result.UnmatchedTokens.Should().BeEquivalentTo("unmatched-token"); } @@ -1571,7 +1573,7 @@ public void Parsed_value_of_empty_string_arg_is_an_empty_string(string arg1, str var result = rootCommand.Parse(new[] { arg1, arg2 }); - result.GetValue(option).Should().BeEmpty(); + GetValue(result, option).Should().BeEmpty(); } } } From df2c8e37767b9d734f7c10bad553e9aa62cb2883 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 28 Feb 2023 18:42:38 +0100 Subject: [PATCH 3/6] implement first version --- ...ommandLine_api_is_not_changed.approved.txt | 1 + src/System.CommandLine/ParseResult.cs | 15 ++++++++-- .../Parsing/CommandResult.cs | 30 +++++++++++++++++++ 3 files changed, 44 insertions(+), 2 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 9688ad3dcf..1593560929 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 @@ -174,6 +174,7 @@ System.CommandLine public System.Collections.Generic.IEnumerable GetCompletions(System.Nullable position = null) public T GetValue(Argument argument) public T GetValue(Option option) + public T GetValue(System.String name) public System.Int32 Invoke(IConsole console = null) public System.Threading.Tasks.Task InvokeAsync(IConsole console = null, System.Threading.CancellationToken cancellationToken = null) public System.String ToString() diff --git a/src/System.CommandLine/ParseResult.cs b/src/System.CommandLine/ParseResult.cs index 4e47e9f093..47c6d02d11 100644 --- a/src/System.CommandLine/ParseResult.cs +++ b/src/System.CommandLine/ParseResult.cs @@ -134,8 +134,19 @@ CommandLineText is null /// /// The name of the Symbol for which to get a value. /// The parsed value or a configured default. - public T? GetValue(string name) => default; - //=> CommandResult.GetValue(name); + /// Thrown when parsing resulted in parsing errors. + public T? GetValue(string name) + { + if (Errors.Count > 0) + { + Throw(Errors); + } + + return CommandResult.GetValue(name); + + static void Throw(IReadOnlyList errors) + => throw new InvalidOperationException(string.Join(Environment.NewLine, errors.Select(e => e.Message))); + } /// public override string ToString() => $"{nameof(ParseResult)}: {this.Diagram()}"; diff --git a/src/System.CommandLine/Parsing/CommandResult.cs b/src/System.CommandLine/Parsing/CommandResult.cs index 1f45067954..7cf7fb4bc9 100644 --- a/src/System.CommandLine/Parsing/CommandResult.cs +++ b/src/System.CommandLine/Parsing/CommandResult.cs @@ -2,6 +2,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System.Collections.Generic; +using System.CommandLine.Binding; using System.CommandLine.Help; using System.Linq; @@ -12,6 +13,8 @@ namespace System.CommandLine.Parsing /// public sealed class CommandResult : SymbolResult { + private Dictionary? _namedResults; + internal CommandResult( Command command, Token token, @@ -41,6 +44,33 @@ internal CommandResult( /// public override string ToString() => $"{nameof(CommandResult)}: {Token.Value} {string.Join(" ", Tokens.Select(t => t.Value))}"; + internal T? GetValue(string name) + { + if (_namedResults is null) + { + Dictionary cache = new (StringComparer.Ordinal); + + foreach (KeyValuePair pair in SymbolResultTree) + { + if (ReferenceEquals(pair.Value.Parent, this)) + { + cache.Add(pair.Key.Name, pair.Value); + } + } + + _namedResults = cache; + } + + _namedResults.TryGetValue(name, out SymbolResult? symbolResult); + + return symbolResult switch + { + ArgumentResult argumentResult => argumentResult.GetValueOrDefault(), + OptionResult optionResult => optionResult.GetValueOrDefault(), + _ => (T?)ArgumentConverter.GetDefaultValue(typeof(T)) + }; + } + internal override bool UseDefaultValueFor(ArgumentResult argumentResult) => argumentResult.Argument.HasDefaultValue && argumentResult.Tokens.Count == 0; From 8a0bb9c407038ecc2b5d4e60db6b23e2dc627928 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 7 Mar 2023 21:29:49 +0100 Subject: [PATCH 4/6] address code review feedback, handle required arguments properly --- .../HostingHandlerTest.cs | 2 +- .../Binding/TypeConversionTests.cs | 8 ++- .../GetValueByNameParserTests.cs | 41 +++++++++----- .../ParserTests.MultipleArguments.cs | 11 ++-- src/System.CommandLine.Tests/ParserTests.cs | 9 ++-- .../ParsingValidationTests.cs | 4 +- src/System.CommandLine/ArgumentArity.cs | 2 +- .../Binding/ArgumentConverter.cs | 2 +- .../LocalizationResources.cs | 12 +++-- src/System.CommandLine/ParseResult.cs | 15 +----- .../Parsing/CommandResult.cs | 54 ++++++++++++------- .../Parsing/SymbolResult.cs | 1 - 12 files changed, 95 insertions(+), 66 deletions(-) diff --git a/src/System.CommandLine.Hosting.Tests/HostingHandlerTest.cs b/src/System.CommandLine.Hosting.Tests/HostingHandlerTest.cs index 3eac19d295..7142339285 100644 --- a/src/System.CommandLine.Hosting.Tests/HostingHandlerTest.cs +++ b/src/System.CommandLine.Hosting.Tests/HostingHandlerTest.cs @@ -206,7 +206,7 @@ public class MyOtherCommand : Command public MyOtherCommand() : base(name: "myothercommand") { Options.Add(new Option("--int-option")); // or nameof(Handler.IntOption).ToKebabCase() if you don't like the string literal - Arguments.Add(new Argument("One")); + Arguments.Add(new Argument("One") { Arity = ArgumentArity.ZeroOrOne }); } public class MyHandler : ICommandHandler diff --git a/src/System.CommandLine.Tests/Binding/TypeConversionTests.cs b/src/System.CommandLine.Tests/Binding/TypeConversionTests.cs index 73dfd09d47..5f8d2d1b88 100644 --- a/src/System.CommandLine.Tests/Binding/TypeConversionTests.cs +++ b/src/System.CommandLine.Tests/Binding/TypeConversionTests.cs @@ -733,8 +733,12 @@ public void When_getting_an_array_of_values_and_specifying_a_conversion_type_tha } [Fact] - public void String_defaults_to_null_when_not_specified() - => GetValue(new Argument("arg"), "").Should().BeNull(); + public void String_defaults_to_null_when_not_specified_only_for_not_required_arguments() + => GetValue( + new Argument("arg") + { + Arity = ArgumentArity.ZeroOrMore + }, "").Should().BeNull(); [Theory] [InlineData(typeof(List))] diff --git a/src/System.CommandLine.Tests/GetValueByNameParserTests.cs b/src/System.CommandLine.Tests/GetValueByNameParserTests.cs index 2e9647cb64..afa5ff82ee 100644 --- a/src/System.CommandLine.Tests/GetValueByNameParserTests.cs +++ b/src/System.CommandLine.Tests/GetValueByNameParserTests.cs @@ -2,7 +2,6 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using FluentAssertions; -using System.Linq; using Xunit; using Xunit.Abstractions; @@ -46,20 +45,20 @@ public void In_case_of_option_name_conflict_the_value_which_belongs_to_the_last_ { RootCommand command = new() { - new Option("opt", new[] { "-i", "--integer" }), + new Option("--integer", "-i"), new Command("inner1") { - new Option("opt", new[] { "-i", "--integer" }), + new Option("--integer", "-i"), new Command("inner2") { - new Option("opt", new[] { "-i", "--integer" }) + new Option("--integer", "-i") } } }; ParseResult parseResult = command.Parse("-i 1 inner1 --integer 2 inner2 -i 3"); - parseResult.GetValue("opt").Should().Be(3); + parseResult.GetValue("--integer").Should().Be(3); } [Fact] @@ -67,20 +66,23 @@ public void When_option_value_is_not_parsed_then_default_value_is_returned() { RootCommand command = new() { - new Option("opt", new[] { "-i", "--integer" }) + new Option("--integer", "-i") }; ParseResult parseResult = command.Parse(""); - parseResult.GetValue("opt").Should().Be(default); + parseResult.GetValue("--integer").Should().Be(default); } [Fact] - public void When_argument_value_is_not_parsed_then_default_value_is_returned() + public void When_optional_argument_is_not_parsed_then_default_value_is_returned() { RootCommand command = new() { new Argument("arg") + { + Arity = ArgumentArity.ZeroOrOne + } }; ParseResult parseResult = command.Parse(""); @@ -93,7 +95,7 @@ public void When_required_option_value_is_not_parsed_then_an_exception_is_thrown { RootCommand command = new() { - new Option("required", new[] { "-i", "--integer" }) + new Option("--required") { IsRequired = true } @@ -101,12 +103,12 @@ public void When_required_option_value_is_not_parsed_then_an_exception_is_thrown ParseResult parseResult = command.Parse(""); - Action getRequired = () => parseResult.GetValue("required"); + Action getRequired = () => parseResult.GetValue("--required"); getRequired .Should() .Throw() - .Where(ex => ex.Message == LocalizationResources.RequiredOptionWasNotProvided("required")); + .Where(ex => ex.Message == LocalizationResources.RequiredOptionWasNotProvided("--required")); } [Fact] @@ -116,7 +118,7 @@ public void When_required_argument_value_is_not_parsed_then_an_exception_is_thro { new Argument("required") { - Arity = new ArgumentArity(1, 1) + Arity = ArgumentArity.ExactlyOne } }; @@ -127,7 +129,20 @@ public void When_required_argument_value_is_not_parsed_then_an_exception_is_thro getRequired .Should() .Throw() - .Where(ex => ex.Message == LocalizationResources.RequiredOptionWasNotProvided("required")); + .Where(ex => ex.Message == LocalizationResources.RequiredArgumentMissing(parseResult.FindResultFor(command.Arguments[0]))); + } + + [Fact] + public void When_non_existing_name_is_used_then_exception_is_thrown() + { + ParseResult parseResult = new Command("noSymbols").Parse(""); + + Action getRequired = () => parseResult.GetValue("required"); + + getRequired + .Should() + .Throw() + .Where(ex => ex.Message == LocalizationResources.RequiredArgumentMissing(parseResult.FindResultFor(command.Arguments[0]))); } } } diff --git a/src/System.CommandLine.Tests/ParserTests.MultipleArguments.cs b/src/System.CommandLine.Tests/ParserTests.MultipleArguments.cs index 2f157b5731..4f080453e9 100644 --- a/src/System.CommandLine.Tests/ParserTests.MultipleArguments.cs +++ b/src/System.CommandLine.Tests/ParserTests.MultipleArguments.cs @@ -257,7 +257,7 @@ public void Unsatisfied_subsequent_argument_with_min_arity_1_parses_as_default_v var result = rootCommand.Parse(""); - result.FindResultFor(arg1).Should().BeNull(); + result.FindResultFor(arg1).Should().NotBeNull(); result.GetValue(arg2).Should().Be("the-default"); } @@ -299,12 +299,9 @@ public void When_there_are_not_enough_tokens_for_all_arguments_then_the_correct_ var result = Parser.Parse(command, providedArgs); - var numberOfMissingArgs = - result - .Errors - .Count(e => e.Message == LocalizationResources.RequiredArgumentMissing(result.CommandResult)); - - numberOfMissingArgs + result + .Errors + .Count .Should() .Be(4 - providedArgs.Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries).Length); } diff --git a/src/System.CommandLine.Tests/ParserTests.cs b/src/System.CommandLine.Tests/ParserTests.cs index 09cc5ddb52..b1f1a8eaeb 100644 --- a/src/System.CommandLine.Tests/ParserTests.cs +++ b/src/System.CommandLine.Tests/ParserTests.cs @@ -1409,7 +1409,7 @@ public void When_command_arguments_are_fewer_than_minimum_arity_then_an_error_is result.Errors .Select(e => e.Message) .Should() - .Contain(LocalizationResources.RequiredArgumentMissing(result.CommandResult)); + .Contain(LocalizationResources.RequiredArgumentMissing(result.FindResultFor(command.Arguments[0]))); } [Fact] @@ -1485,7 +1485,10 @@ public void Option_argument_arity_can_be_a_range_with_a_lower_bound_greater_than [Fact] public void When_option_arguments_are_fewer_than_minimum_arity_then_an_error_is_returned() { - var option = new Option("-x") { Arity = new ArgumentArity(2, 3) }; + var option = new Option("-x") + { + Arity = new ArgumentArity(2, 3) + }; var command = new Command("the-command") { @@ -1497,7 +1500,7 @@ public void When_option_arguments_are_fewer_than_minimum_arity_then_an_error_is_ result.Errors .Select(e => e.Message) .Should() - .Contain(LocalizationResources.RequiredArgumentMissing(result.CommandResult.FindResultFor(option))); + .Contain(LocalizationResources.RequiredArgumentMissing(result.FindResultFor(option))); } [Fact] diff --git a/src/System.CommandLine.Tests/ParsingValidationTests.cs b/src/System.CommandLine.Tests/ParsingValidationTests.cs index 07119ea452..ea4f95b033 100644 --- a/src/System.CommandLine.Tests/ParsingValidationTests.cs +++ b/src/System.CommandLine.Tests/ParsingValidationTests.cs @@ -236,7 +236,7 @@ public void When_a_required_option_is_not_supplied_then_an_error_is_returned() .Should() .HaveCount(1) .And - .Contain(e => ((CommandResult)e.SymbolResult).Command == command) + .ContainSingle() .Which .Message .Should() @@ -260,7 +260,7 @@ public void When_a_required_option_has_multiple_aliases_the_error_message_uses_t .Should() .HaveCount(1) .And - .Contain(e => ((CommandResult)e.SymbolResult).Command == command) + .ContainSingle() .Which .Message .Should() diff --git a/src/System.CommandLine/ArgumentArity.cs b/src/System.CommandLine/ArgumentArity.cs index dbf9a4171e..f65e8b4f0a 100644 --- a/src/System.CommandLine/ArgumentArity.cs +++ b/src/System.CommandLine/ArgumentArity.cs @@ -92,7 +92,7 @@ internal static bool Validate(ArgumentResult argumentResult, [NotNullWhen(false) error = ArgumentConversionResult.Failure( argumentResult, - LocalizationResources.RequiredArgumentMissing(argumentResult.Parent), + LocalizationResources.RequiredArgumentMissing(argumentResult), ArgumentConversionResultType.FailedMissingArgument); return false; diff --git a/src/System.CommandLine/Binding/ArgumentConverter.cs b/src/System.CommandLine/Binding/ArgumentConverter.cs index 679111c4a5..cb2f952b94 100644 --- a/src/System.CommandLine/Binding/ArgumentConverter.cs +++ b/src/System.CommandLine/Binding/ArgumentConverter.cs @@ -186,7 +186,7 @@ ArgumentConversionResultType.NoArgument when conversionResult.ArgumentResult.Arg ArgumentConversionResultType.NoArgument when conversionResult.ArgumentResult.Argument.Arity.MinimumNumberOfValues > 0 => Failure( conversionResult.ArgumentResult, - LocalizationResources.RequiredArgumentMissing(conversionResult.ArgumentResult.Parent!), + LocalizationResources.RequiredArgumentMissing(conversionResult.ArgumentResult), ArgumentConversionResultType.FailedMissingArgument), _ => conversionResult diff --git a/src/System.CommandLine/LocalizationResources.cs b/src/System.CommandLine/LocalizationResources.cs index 3e0fe99dee..6584030713 100644 --- a/src/System.CommandLine/LocalizationResources.cs +++ b/src/System.CommandLine/LocalizationResources.cs @@ -52,10 +52,16 @@ internal static string InvalidCharactersInFileName(char invalidChar) => /// /// Interpolates values into a localized string similar to Required argument missing for command: {0}. /// - internal static string RequiredArgumentMissing(SymbolResult symbolResult) => - symbolResult is CommandResult commandResult + internal static string RequiredArgumentMissing(ArgumentResult argumentResult) => + argumentResult.Parent is CommandResult commandResult ? GetResourceString(Properties.Resources.CommandRequiredArgumentMissing, commandResult.Token.Value) - : GetResourceString(Properties.Resources.OptionRequiredArgumentMissing, GetOptionName((OptionResult)symbolResult)); + : RequiredArgumentMissing((OptionResult)argumentResult.Parent!); + + /// + /// Interpolates values into a localized string similar to Required argument missing for option: {0}. + /// + internal static string RequiredArgumentMissing(OptionResult optionResult) => + GetResourceString(Properties.Resources.OptionRequiredArgumentMissing, GetOptionName(optionResult)); /// /// Interpolates values into a localized string similar to Required command was not provided. diff --git a/src/System.CommandLine/ParseResult.cs b/src/System.CommandLine/ParseResult.cs index 47c6d02d11..bb11d8c013 100644 --- a/src/System.CommandLine/ParseResult.cs +++ b/src/System.CommandLine/ParseResult.cs @@ -134,19 +134,8 @@ CommandLineText is null /// /// The name of the Symbol for which to get a value. /// The parsed value or a configured default. - /// Thrown when parsing resulted in parsing errors. - public T? GetValue(string name) - { - if (Errors.Count > 0) - { - Throw(Errors); - } - - return CommandResult.GetValue(name); - - static void Throw(IReadOnlyList errors) - => throw new InvalidOperationException(string.Join(Environment.NewLine, errors.Select(e => e.Message))); - } + /// Thrown when parsing resulted in parse error(s). + public T? GetValue(string name) => CommandResult.GetValue(name); /// public override string ToString() => $"{nameof(ParseResult)}: {this.Diagram()}"; diff --git a/src/System.CommandLine/Parsing/CommandResult.cs b/src/System.CommandLine/Parsing/CommandResult.cs index 7cf7fb4bc9..ac3d1024b0 100644 --- a/src/System.CommandLine/Parsing/CommandResult.cs +++ b/src/System.CommandLine/Parsing/CommandResult.cs @@ -13,7 +13,7 @@ namespace System.CommandLine.Parsing /// public sealed class CommandResult : SymbolResult { - private Dictionary? _namedResults; + private Dictionary? _namedResults; internal CommandResult( Command command, @@ -48,20 +48,34 @@ internal CommandResult( { if (_namedResults is null) { - Dictionary cache = new (StringComparer.Ordinal); + // A null value means that given name exists, but was not parsed + Dictionary cache = new (StringComparer.Ordinal); - foreach (KeyValuePair pair in SymbolResultTree) + if (Command.HasArguments) { - if (ReferenceEquals(pair.Value.Parent, this)) + for (int i = 0; i < Command.Arguments.Count; i++) { - cache.Add(pair.Key.Name, pair.Value); + SymbolResultTree.TryGetValue(Command.Arguments[i], out SymbolResult? parsedResult); + cache.Add(Command.Arguments[i].Name, parsedResult); + } + } + + if (Command.HasOptions) + { + for (int i = 0; i < Command.Options.Count; i++) + { + SymbolResultTree.TryGetValue(Command.Options[i], out SymbolResult? parsedResult); + cache.Add(Command.Options[i].Name, parsedResult); } } _namedResults = cache; } - _namedResults.TryGetValue(name, out SymbolResult? symbolResult); + if (!_namedResults.TryGetValue(name, out SymbolResult? symbolResult)) + { + throw new InvalidOperationException($"No symbol result found for \"{name}\" for command {Command.Name}."); + } return symbolResult switch { @@ -118,7 +132,7 @@ private void ValidateOptions(bool completeValidation) { var option = options[i]; - if (!completeValidation && !(option.AppliesToSelfAndChildren || option.Argument.HasDefaultValue || (option is HelpOption or VersionOption))) + if (!completeValidation && !(option.AppliesToSelfAndChildren || option.Argument.HasDefaultValue || option is VersionOption)) { continue; } @@ -128,18 +142,19 @@ private void ValidateOptions(bool completeValidation) if (!SymbolResultTree.TryGetValue(option, out SymbolResult? symbolResult)) { - if (option.IsRequired) - { - AddError(LocalizationResources.RequiredOptionWasNotProvided(option.Name)); - continue; - } - else if (option.Argument.HasDefaultValue) + if (option.IsRequired || option.Argument.HasDefaultValue) { optionResult = new(option, SymbolResultTree, null, this); SymbolResultTree.Add(optionResult.Option, optionResult); argumentResult = new(optionResult.Option.Argument, SymbolResultTree, optionResult); SymbolResultTree.Add(optionResult.Option.Argument, argumentResult); + + if (option.IsRequired && !option.Argument.HasDefaultValue) + { + argumentResult.AddError(LocalizationResources.RequiredOptionWasNotProvided(option.Name)); + continue; + } } else { @@ -195,15 +210,16 @@ private void ValidateArguments(bool completeValidation) { argumentResult = (ArgumentResult)symbolResult; } - else if (argument.HasDefaultValue) + else if (argument.HasDefaultValue || argument.Arity.MinimumNumberOfValues > 0) { argumentResult = new ArgumentResult(argument, SymbolResultTree, this); SymbolResultTree[argument] = argumentResult; - } - else if (argument.Arity.MinimumNumberOfValues > 0) - { - AddError(LocalizationResources.RequiredArgumentMissing(this)); - continue; + + if (!argument.HasDefaultValue && argument.Arity.MinimumNumberOfValues > 0) + { + argumentResult.AddError(LocalizationResources.RequiredArgumentMissing(argumentResult)); + continue; + } } else { diff --git a/src/System.CommandLine/Parsing/SymbolResult.cs b/src/System.CommandLine/Parsing/SymbolResult.cs index fcebe95aae..3178d7e81c 100644 --- a/src/System.CommandLine/Parsing/SymbolResult.cs +++ b/src/System.CommandLine/Parsing/SymbolResult.cs @@ -3,7 +3,6 @@ using System.Collections.Generic; using System.CommandLine.Binding; -using System.Linq; namespace System.CommandLine.Parsing { From 4fed9625c41d4584e58f99e919422e98c457ef0e Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 9 Mar 2023 20:05:43 +0100 Subject: [PATCH 5/6] add missing tests, throw NotSupportedException for duplicates --- .../GetValueByNameParserTests.cs | 80 ++++++++++++++++++- .../Parsing/CommandResult.cs | 28 ++++--- 2 files changed, 94 insertions(+), 14 deletions(-) diff --git a/src/System.CommandLine.Tests/GetValueByNameParserTests.cs b/src/System.CommandLine.Tests/GetValueByNameParserTests.cs index afa5ff82ee..c83eea7046 100644 --- a/src/System.CommandLine.Tests/GetValueByNameParserTests.cs +++ b/src/System.CommandLine.Tests/GetValueByNameParserTests.cs @@ -135,14 +135,88 @@ public void When_required_argument_value_is_not_parsed_then_an_exception_is_thro [Fact] public void When_non_existing_name_is_used_then_exception_is_thrown() { - ParseResult parseResult = new Command("noSymbols").Parse(""); + const string nonExistingName = "nonExisting"; + Command command = new ("noSymbols"); + ParseResult parseResult = command.Parse(""); - Action getRequired = () => parseResult.GetValue("required"); + Action getRequired = () => parseResult.GetValue(nonExistingName); getRequired .Should() .Throw() - .Where(ex => ex.Message == LocalizationResources.RequiredArgumentMissing(parseResult.FindResultFor(command.Arguments[0]))); + .Where(ex => ex.Message == $"No symbol result found for \"{nonExistingName}\" for command \"{command.Name}\"."); + } + + [Fact] + public void When_an_option_and_argument_use_same_name_on_the_same_level_of_the_tree_an_exception_is_thrown() + { + const string sameName = "same"; + + RootCommand command = new() + { + new Argument(sameName) + { + Arity = ArgumentArity.ZeroOrOne + }, + new Option(sameName) + }; + + ParseResult parseResult = command.Parse(""); + + Action getConflicted = () => parseResult.GetValue(sameName); + + getConflicted + .Should() + .Throw() + .Where(ex => ex.Message == $"More than one symbol uses name \"{sameName}\" for command \"{command.Name}\"."); + } + + [Fact] + public void When_an_option_and_argument_use_same_name_on_different_levels_of_the_tree_the_value_which_belongs_to_parsed_command_is_returned() + { + const string sameName = "same"; + + Command command = new("outer") + { + new Argument(sameName), + new Command("inner") + { + new Option(sameName) + } + }; + + ParseResult parseResult = command.Parse($"outer 123 inner {sameName} 456"); + parseResult.GetValue(sameName).Should().Be(456); + + parseResult = command.Parse($"outer 123"); + parseResult.GetValue(sameName).Should().Be(123); + } + + [Fact] + public void When_an_option_and_argument_use_same_name_on_different_levels_of_the_tree_the_default_value_which_belongs_to_parsed_command_is_returned() + { + const string sameName = "same"; + + Command command = new("outer") + { + new Argument(sameName) + { + DefaultValueFactory = (_) => 123 + }, + new Command("inner") + { + new Option(sameName) + { + DefaultValueFactory = (_) => 456 + } + } + }; + + ParseResult parseResult = command.Parse($"outer inner 456"); + parseResult.GetValue(sameName).Should().Be(456); + + parseResult = command.Parse($"outer 123"); + parseResult.GetValue(sameName).Should().Be(123); } } } diff --git a/src/System.CommandLine/Parsing/CommandResult.cs b/src/System.CommandLine/Parsing/CommandResult.cs index ac3d1024b0..0f7d5591cb 100644 --- a/src/System.CommandLine/Parsing/CommandResult.cs +++ b/src/System.CommandLine/Parsing/CommandResult.cs @@ -53,20 +53,12 @@ internal CommandResult( if (Command.HasArguments) { - for (int i = 0; i < Command.Arguments.Count; i++) - { - SymbolResultTree.TryGetValue(Command.Arguments[i], out SymbolResult? parsedResult); - cache.Add(Command.Arguments[i].Name, parsedResult); - } + Populate(cache, Command.Arguments); } if (Command.HasOptions) { - for (int i = 0; i < Command.Options.Count; i++) - { - SymbolResultTree.TryGetValue(Command.Options[i], out SymbolResult? parsedResult); - cache.Add(Command.Options[i].Name, parsedResult); - } + Populate(cache, Command.Options); } _namedResults = cache; @@ -74,7 +66,7 @@ internal CommandResult( if (!_namedResults.TryGetValue(name, out SymbolResult? symbolResult)) { - throw new InvalidOperationException($"No symbol result found for \"{name}\" for command {Command.Name}."); + throw new InvalidOperationException($"No symbol result found for \"{name}\" for command \"{Command.Name}\"."); } return symbolResult switch @@ -83,6 +75,20 @@ internal CommandResult( OptionResult optionResult => optionResult.GetValueOrDefault(), _ => (T?)ArgumentConverter.GetDefaultValue(typeof(T)) }; + + void Populate(Dictionary cache, IList symbols) where TSymbol : Symbol + { + for (int i = 0; i < symbols.Count; i++) + { + if (cache.ContainsKey(symbols[i].Name)) + { + throw new NotSupportedException($"More than one symbol uses name \"{symbols[i].Name}\" for command \"{Command.Name}\"."); + } + + SymbolResultTree.TryGetValue(symbols[i], out SymbolResult? parsedSymbol); + cache.Add(symbols[i].Name, parsedSymbol); + } + } } internal override bool UseDefaultValueFor(ArgumentResult argumentResult) From 82f24bd3c2665d5995dff8eaedcc8146e8004073 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 13 Mar 2023 14:46:06 +0100 Subject: [PATCH 6/6] address code review feedback: * add test coverage for casting T to U, fix discovered issue * throw ArgumentException instead InvalidOperationException when no symbol is found for given name * move the method from CommandResult to ParseResult to make it clear that it's available only after parsing has finished --- .../GetValueByNameParserTests.cs | 70 ++++++++++++++++++- src/System.CommandLine/ParseResult.cs | 68 +++++++++++++++++- .../Parsing/CommandResult.cs | 50 ------------- 3 files changed, 135 insertions(+), 53 deletions(-) diff --git a/src/System.CommandLine.Tests/GetValueByNameParserTests.cs b/src/System.CommandLine.Tests/GetValueByNameParserTests.cs index c83eea7046..a22f5d860e 100644 --- a/src/System.CommandLine.Tests/GetValueByNameParserTests.cs +++ b/src/System.CommandLine.Tests/GetValueByNameParserTests.cs @@ -2,6 +2,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using FluentAssertions; +using System.Collections.Generic; using Xunit; using Xunit.Abstractions; @@ -143,7 +144,7 @@ public void When_non_existing_name_is_used_then_exception_is_thrown() getRequired .Should() - .Throw() + .Throw() .Where(ex => ex.Message == $"No symbol result found for \"{nonExistingName}\" for command \"{command.Name}\"."); } @@ -218,5 +219,72 @@ public void When_an_option_and_argument_use_same_name_on_different_levels_of_the parseResult = command.Parse($"outer 123"); parseResult.GetValue(sameName).Should().Be(123); } + + [Fact] + public void T_can_be_casted_to_nullable_of_T() + { + RootCommand command = new() + { + new Argument("name") + }; + + ParseResult parseResult = command.Parse("123"); + + parseResult.GetValue("name").Should().Be(123); + } + + [Fact] + public void Array_of_T_can_be_casted_to_ienumerable_of_T() + { + RootCommand command = new() + { + new Argument("name") + }; + + ParseResult parseResult = command.Parse("1 2 3"); + + parseResult.GetValue>("name").Should().BeEquivalentTo(new int[] { 1, 2, 3 }); + } + + [Fact] + public void When_casting_is_not_allowed_an_exception_is_thrown() + { + const string Name = "name"; + + RootCommand command = new() + { + new Argument(Name) + }; + + ParseResult parseResult = command.Parse("123"); + + Assert(() => parseResult.GetValue(Name)); + Assert(() => parseResult.GetValue(Name)); + Assert(() => parseResult.GetValue(Name)); + + static void Assert(Action invalidCast) + => invalidCast.Should().Throw(); + } + + [Fact] + public void Parse_errors_have_precedence_over_type_mismatch() + { + RootCommand command = new() + { + new Option("--required") + { + IsRequired = true + } + }; + + ParseResult parseResult = command.Parse(""); + + Action getRequiredWithTypeMismatch = () => parseResult.GetValue("--required"); + + getRequiredWithTypeMismatch + .Should() + .Throw() + .Where(ex => ex.Message == LocalizationResources.RequiredOptionWasNotProvided("--required")); + } } } diff --git a/src/System.CommandLine/ParseResult.cs b/src/System.CommandLine/ParseResult.cs index bb11d8c013..1476ef3297 100644 --- a/src/System.CommandLine/ParseResult.cs +++ b/src/System.CommandLine/ParseResult.cs @@ -22,6 +22,7 @@ public class ParseResult private readonly IReadOnlyList _unmatchedTokens; private CompletionContext? _completionContext; private ICommandHandler? _handler; + private Dictionary? _namedResults; internal ParseResult( CommandLineConfiguration configuration, @@ -130,12 +131,75 @@ CommandLineText is null => RootCommandResult.GetValue(option); /// - /// Gets the parsed or default value for the specified symbol name, in the context of parsed command. + /// Gets the parsed or default value for the specified symbol name, in the context of parsed command (not entire symbol tree). /// /// The name of the Symbol for which to get a value. /// The parsed value or a configured default. /// Thrown when parsing resulted in parse error(s). - public T? GetValue(string name) => CommandResult.GetValue(name); + /// Thrown when there was no symbol defined for given name for the parsed command. + /// Thrown when parsed result can not be casted to . + public T? GetValue(string name) + { + var command = CommandResult.Command; + if (_namedResults is null) + { + // A null value means that given name exists, but was not parsed + Dictionary cache = new(StringComparer.Ordinal); + + if (command.HasArguments) + { + Populate(cache, command.Arguments); + } + + if (command.HasOptions) + { + Populate(cache, command.Options); + } + + _namedResults = cache; + } + + if (!_namedResults.TryGetValue(name, out SymbolResult? symbolResult)) + { + throw new ArgumentException($"No symbol result found for \"{name}\" for command \"{command.Name}\"."); + } + + return symbolResult switch + { + ArgumentResult argumentResult => Convert(argumentResult.GetArgumentConversionResult()), + OptionResult optionResult => Convert(optionResult.ArgumentConversionResult), + _ => (T?)ArgumentConverter.GetDefaultValue(typeof(T)) + }; + + void Populate(Dictionary cache, IList symbols) where TSymbol : Symbol + { + var symbolResultTree = CommandResult.SymbolResultTree; + for (int i = 0; i < symbols.Count; i++) + { + if (cache.ContainsKey(symbols[i].Name)) + { + throw new NotSupportedException($"More than one symbol uses name \"{symbols[i].Name}\" for command \"{command.Name}\"."); + } + + symbolResultTree.TryGetValue(symbols[i], out SymbolResult? parsedSymbol); + cache.Add(symbols[i].Name, parsedSymbol); + } + } + + static T? Convert(ArgumentConversionResult validatedResult) + { + var convertedResult = validatedResult.ConvertIfNeeded(typeof(T)); + + if (validatedResult.Result == ArgumentConversionResultType.Successful + && convertedResult.Result == ArgumentConversionResultType.NoArgument) + { + // invalid cast has been detected, InvalidCastException will be thrown + return (T)validatedResult.Value!; + } + + return convertedResult.GetValueOrDefault(); + } + } /// public override string ToString() => $"{nameof(ParseResult)}: {this.Diagram()}"; diff --git a/src/System.CommandLine/Parsing/CommandResult.cs b/src/System.CommandLine/Parsing/CommandResult.cs index 0f7d5591cb..5c9c082641 100644 --- a/src/System.CommandLine/Parsing/CommandResult.cs +++ b/src/System.CommandLine/Parsing/CommandResult.cs @@ -2,7 +2,6 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System.Collections.Generic; -using System.CommandLine.Binding; using System.CommandLine.Help; using System.Linq; @@ -13,8 +12,6 @@ namespace System.CommandLine.Parsing /// public sealed class CommandResult : SymbolResult { - private Dictionary? _namedResults; - internal CommandResult( Command command, Token token, @@ -44,53 +41,6 @@ internal CommandResult( /// public override string ToString() => $"{nameof(CommandResult)}: {Token.Value} {string.Join(" ", Tokens.Select(t => t.Value))}"; - internal T? GetValue(string name) - { - if (_namedResults is null) - { - // A null value means that given name exists, but was not parsed - Dictionary cache = new (StringComparer.Ordinal); - - if (Command.HasArguments) - { - Populate(cache, Command.Arguments); - } - - if (Command.HasOptions) - { - Populate(cache, Command.Options); - } - - _namedResults = cache; - } - - if (!_namedResults.TryGetValue(name, out SymbolResult? symbolResult)) - { - throw new InvalidOperationException($"No symbol result found for \"{name}\" for command \"{Command.Name}\"."); - } - - return symbolResult switch - { - ArgumentResult argumentResult => argumentResult.GetValueOrDefault(), - OptionResult optionResult => optionResult.GetValueOrDefault(), - _ => (T?)ArgumentConverter.GetDefaultValue(typeof(T)) - }; - - void Populate(Dictionary cache, IList symbols) where TSymbol : Symbol - { - for (int i = 0; i < symbols.Count; i++) - { - if (cache.ContainsKey(symbols[i].Name)) - { - throw new NotSupportedException($"More than one symbol uses name \"{symbols[i].Name}\" for command \"{Command.Name}\"."); - } - - SymbolResultTree.TryGetValue(symbols[i], out SymbolResult? parsedSymbol); - cache.Add(symbols[i].Name, parsedSymbol); - } - } - } - internal override bool UseDefaultValueFor(ArgumentResult argumentResult) => argumentResult.Argument.HasDefaultValue && argumentResult.Tokens.Count == 0;