From 8f75d4d02b90da7aab76baf37731f3547b9da557 Mon Sep 17 00:00:00 2001 From: Nate McMaster Date: Sat, 7 Nov 2020 14:31:24 -0800 Subject: [PATCH] cleanup: make .Values read-only on CommandArgument/Option Exposing a raw List of values on CommandArgument and CommandOption has made it difficult to cleanly implement features like default values or parsing. This changes the return type of .Values to a read-only list, and adds TryParse for adding new values, and Reset for clearing them. --- src/CommandLineUtils/CommandArgument.cs | 38 ++++++++++++++----- .../CommandLineApplication.cs | 1 - src/CommandLineUtils/CommandOption.cs | 20 ++++++---- .../Internal/CommandLineProcessor.cs | 2 +- src/CommandLineUtils/PublicAPI.Shipped.txt | 4 +- src/CommandLineUtils/PublicAPI.Unshipped.txt | 4 ++ .../Validation/AttributeValidator.cs | 2 +- .../AttributeValidatorTests.cs | 8 ++-- .../CommandLineApplicationTests.cs | 4 +- .../DefaultHelpTextGeneratorTests.cs | 10 ++--- .../ResponseFileTests.cs | 2 +- 11 files changed, 60 insertions(+), 35 deletions(-) diff --git a/src/CommandLineUtils/CommandArgument.cs b/src/CommandLineUtils/CommandArgument.cs index e218de2f..faf0a06b 100644 --- a/src/CommandLineUtils/CommandArgument.cs +++ b/src/CommandLineUtils/CommandArgument.cs @@ -16,13 +16,7 @@ namespace McMaster.Extensions.CommandLineUtils /// public class CommandArgument { - /// - /// Initializes a new instance of . - /// - public CommandArgument() - { - Values = new List(); - } + private protected List _values = new List(); /// /// The name of the argument. @@ -42,7 +36,7 @@ public CommandArgument() /// /// All values specified, if any. /// - public List Values { get; private set; } + public IReadOnlyList Values => _values; /// /// Allow multiple values. @@ -54,6 +48,11 @@ public CommandArgument() /// public string? Value => Values.FirstOrDefault(); + /// + /// True if this argument has been assigned values. + /// + public bool HasValue => Values.Any(); + /// /// A collection of validators that execute before invoking . /// When validation fails, is invoked. @@ -65,9 +64,28 @@ public CommandArgument() /// internal Type UnderlyingType { get; set; } - internal void Reset() + /// + /// Try to add a value to this argument. + /// + /// The argument value to be added. + /// True if the value was accepted, false if the value cannot be added. + public bool TryParse(string? value) + { + if (_values.Count == 1 && !MultipleValues) + { + return false; + } + + _values.Add(value); + return true; + } + + /// + /// Clear any parsed values from this argument. + /// + public virtual void Reset() { - Values.Clear(); + _values.Clear(); } } } diff --git a/src/CommandLineUtils/CommandLineApplication.cs b/src/CommandLineUtils/CommandLineApplication.cs index 38e6fd60..85a0fc36 100644 --- a/src/CommandLineUtils/CommandLineApplication.cs +++ b/src/CommandLineUtils/CommandLineApplication.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.ComponentModel; using System.ComponentModel.DataAnnotations; using System.IO; using System.Linq; diff --git a/src/CommandLineUtils/CommandOption.cs b/src/CommandLineUtils/CommandOption.cs index 1a0c2bb7..c82ba6a4 100644 --- a/src/CommandLineUtils/CommandOption.cs +++ b/src/CommandLineUtils/CommandOption.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; -using System.ComponentModel; using System.Linq; using System.Text; using McMaster.Extensions.CommandLineUtils.Validation; @@ -17,6 +16,8 @@ namespace McMaster.Extensions.CommandLineUtils /// public class CommandOption { + private protected readonly List _values = new List(); + /// /// Initializes a new . /// @@ -99,7 +100,7 @@ internal CommandOption(CommandOptionType type) /// /// Any values found during parsing, if any. /// - public List Values { get; } = new List(); + public IReadOnlyList Values => _values; /// /// Defines the type of the option. @@ -138,15 +139,15 @@ public bool TryParse(string? value) switch (OptionType) { case CommandOptionType.MultipleValue: - Values.Add(value); + _values.Add(value); break; case CommandOptionType.SingleOrNoValue: case CommandOptionType.SingleValue: - if (Values.Any()) + if (_values.Any()) { return false; } - Values.Add(value); + _values.Add(value); break; case CommandOptionType.NoValue: if (value != null) @@ -156,7 +157,7 @@ public bool TryParse(string? value) // Add a value so .HasValue() == true // Also, the count can be used to determine how many times a flag was specified - Values.Add(null); + _values.Add(null); break; default: throw new NotImplementedException(); @@ -235,9 +236,12 @@ private bool IsEnglishLetter(char c) return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z'); } - internal void Reset() + /// + /// Clear any parsed values from this argument. + /// + public virtual void Reset() { - Values.Clear(); + _values.Clear(); } } } diff --git a/src/CommandLineUtils/Internal/CommandLineProcessor.cs b/src/CommandLineUtils/Internal/CommandLineProcessor.cs index 7a19722f..120803af 100644 --- a/src/CommandLineUtils/Internal/CommandLineProcessor.cs +++ b/src/CommandLineUtils/Internal/CommandLineProcessor.cs @@ -125,7 +125,7 @@ private bool ProcessCommandOrParameter(CommandOrParameterArgument arg) if (_currentCommandArguments.MoveNext()) { - _currentCommandArguments.Current.Values.Add(arg.Raw); + _currentCommandArguments.Current.TryParse(arg.Raw); return true; } diff --git a/src/CommandLineUtils/PublicAPI.Shipped.txt b/src/CommandLineUtils/PublicAPI.Shipped.txt index d004e9d9..7ceb21b6 100644 --- a/src/CommandLineUtils/PublicAPI.Shipped.txt +++ b/src/CommandLineUtils/PublicAPI.Shipped.txt @@ -63,7 +63,7 @@ McMaster.Extensions.CommandLineUtils.CommandArgument.ShowInHelpText.get -> bool McMaster.Extensions.CommandLineUtils.CommandArgument.ShowInHelpText.set -> void McMaster.Extensions.CommandLineUtils.CommandArgument.Validators.get -> System.Collections.Generic.ICollection! McMaster.Extensions.CommandLineUtils.CommandArgument.Value.get -> string? -McMaster.Extensions.CommandLineUtils.CommandArgument.Values.get -> System.Collections.Generic.List! +McMaster.Extensions.CommandLineUtils.CommandArgument.Values.get -> System.Collections.Generic.IReadOnlyList! McMaster.Extensions.CommandLineUtils.CommandArgument McMaster.Extensions.CommandLineUtils.CommandArgument.CommandArgument(McMaster.Extensions.CommandLineUtils.Abstractions.IValueParser! valueParser) -> void McMaster.Extensions.CommandLineUtils.CommandArgument.ParsedValue.get -> T @@ -213,7 +213,7 @@ McMaster.Extensions.CommandLineUtils.CommandOption.Validators.get -> System.Coll McMaster.Extensions.CommandLineUtils.CommandOption.Value() -> string? McMaster.Extensions.CommandLineUtils.CommandOption.ValueName.get -> string? McMaster.Extensions.CommandLineUtils.CommandOption.ValueName.set -> void -McMaster.Extensions.CommandLineUtils.CommandOption.Values.get -> System.Collections.Generic.List! +McMaster.Extensions.CommandLineUtils.CommandOption.Values.get -> System.Collections.Generic.IReadOnlyList! McMaster.Extensions.CommandLineUtils.CommandOption McMaster.Extensions.CommandLineUtils.CommandOption.CommandOption(McMaster.Extensions.CommandLineUtils.Abstractions.IValueParser! valueParser, string! template, McMaster.Extensions.CommandLineUtils.CommandOptionType optionType) -> void McMaster.Extensions.CommandLineUtils.CommandOption.ParsedValue.get -> T diff --git a/src/CommandLineUtils/PublicAPI.Unshipped.txt b/src/CommandLineUtils/PublicAPI.Unshipped.txt index 7dc5c581..c5893cb1 100644 --- a/src/CommandLineUtils/PublicAPI.Unshipped.txt +++ b/src/CommandLineUtils/PublicAPI.Unshipped.txt @@ -1 +1,5 @@ #nullable enable +McMaster.Extensions.CommandLineUtils.CommandArgument.HasValue.get -> bool +McMaster.Extensions.CommandLineUtils.CommandArgument.TryParse(string? value) -> bool +virtual McMaster.Extensions.CommandLineUtils.CommandArgument.Reset() -> void +virtual McMaster.Extensions.CommandLineUtils.CommandOption.Reset() -> void diff --git a/src/CommandLineUtils/Validation/AttributeValidator.cs b/src/CommandLineUtils/Validation/AttributeValidator.cs index b0da6ac2..565b6ab9 100644 --- a/src/CommandLineUtils/Validation/AttributeValidator.cs +++ b/src/CommandLineUtils/Validation/AttributeValidator.cs @@ -52,7 +52,7 @@ public ValidationResult GetValidationResult(CommandOption option, ValidationCont public ValidationResult GetValidationResult(CommandArgument argument, ValidationContext context) => GetValidationResult(argument.Values, context); - private ValidationResult GetValidationResult(List? values, ValidationContext context) + private ValidationResult GetValidationResult(IReadOnlyList? values, ValidationContext context) { if (values == null) { diff --git a/test/CommandLineUtils.Tests/AttributeValidatorTests.cs b/test/CommandLineUtils.Tests/AttributeValidatorTests.cs index 1256d8f3..ddbb9f94 100644 --- a/test/CommandLineUtils.Tests/AttributeValidatorTests.cs +++ b/test/CommandLineUtils.Tests/AttributeValidatorTests.cs @@ -29,7 +29,7 @@ public void ItOnlyInvokesAttributeIfValueExists() Assert.Equal(ValidationResult.Success, validator.GetValidationResult(arg, context)); - arg.Values.Add(null); + arg.TryParse(null); Assert.Throws(() => validator.GetValidationResult(arg, context)); } @@ -46,12 +46,12 @@ public void ItExecutesValidationAttribute(Type attributeType, string validValue, var factory = new CommandLineValidationContextFactory(app); var context = factory.Create(arg); - arg.Values.Add(validValue); + arg.TryParse(validValue); Assert.Equal(ValidationResult.Success, validator.GetValidationResult(arg, context)); - arg.Values.Clear(); - arg.Values.Add(invalidValue); + arg.Reset(); + arg.TryParse(invalidValue); var result = validator.GetValidationResult(arg, context); Assert.NotNull(result); Assert.NotEmpty(result.ErrorMessage); diff --git a/test/CommandLineUtils.Tests/CommandLineApplicationTests.cs b/test/CommandLineUtils.Tests/CommandLineApplicationTests.cs index e7a0c759..9e2fba19 100644 --- a/test/CommandLineUtils.Tests/CommandLineApplicationTests.cs +++ b/test/CommandLineUtils.Tests/CommandLineApplicationTests.cs @@ -655,7 +655,7 @@ public void OptionsWithSameName() Assert.Equal("top", top.Value()); Assert.Null(nested?.Value()); - top.Values.Clear(); + top.Reset(); app.Execute("subcmd", "-a", "nested"); Assert.Null(top.Value()); @@ -765,7 +765,7 @@ public void OptionSeparatorMustNotUseSpace() app.Parse("--debug:hive", "abc")); Assert.Equal("Missing value for option 'debug:hive'", ex.Message); - option.Values.Clear(); + option.Reset(); app.Parse("--debug:hive=abc"); Assert.Equal("abc", option.Value()); } diff --git a/test/CommandLineUtils.Tests/DefaultHelpTextGeneratorTests.cs b/test/CommandLineUtils.Tests/DefaultHelpTextGeneratorTests.cs index 1a0c3df2..ed1e982a 100644 --- a/test/CommandLineUtils.Tests/DefaultHelpTextGeneratorTests.cs +++ b/test/CommandLineUtils.Tests/DefaultHelpTextGeneratorTests.cs @@ -242,10 +242,10 @@ public class MyApp } [Theory] - [InlineData("-h", "-h", " -h Show help information.", " Subcommand ")] - [InlineData("--help", "--help", " --help Show help information.", " Subcommand ")] - [InlineData("-?", "-?", " -? Show help information.", " Subcommand ")] - [InlineData(null, "-?|-h|--help", " -?|-h|--help Show help information.", " Subcommand ")] + [InlineData("-h", "-h", " -h Show help information.", " Subcommand ")] + [InlineData("--help", "--help", " --help Show help information.", " Subcommand ")] + [InlineData("-?", "-?", " -? Show help information.", " Subcommand ")] + [InlineData(null, "-?|-h|--help", " -?|-h|--help Show help information.", " Subcommand ")] public void ShowHelpWithSubcommands(string helpOption, string expectedHintText, string expectedOptionsText, string expectedCommandsText) { @@ -261,7 +261,7 @@ public void ShowHelpWithSubcommands(string helpOption, string expectedHintText, {expectedOptionsText} Commands: -{expectedCommandsText} +{expectedCommandsText} Run 'test [command] {expectedHintText}' for more information about a command. diff --git a/test/CommandLineUtils.Tests/ResponseFileTests.cs b/test/CommandLineUtils.Tests/ResponseFileTests.cs index abd26442..90621e12 100644 --- a/test/CommandLineUtils.Tests/ResponseFileTests.cs +++ b/test/CommandLineUtils.Tests/ResponseFileTests.cs @@ -30,7 +30,7 @@ private string CreateResponseFile(params string[] lines) return rsp; } - private List ParseResponseFile(ResponseFileHandling options, params string[] responseFileLines) + private IReadOnlyList ParseResponseFile(ResponseFileHandling options, params string[] responseFileLines) { var rsp = CreateResponseFile(responseFileLines); var app = new CommandLineApplication