From c0287d79cda3bbfbb1825bdbde7a8d0d7ff090eb Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 12 Dec 2022 21:37:30 +0100 Subject: [PATCH 1/7] remove IdentifierSymbol._specifiedName, just use Symbol._name --- ....System_CommandLine_api_is_not_changed.approved.txt | 1 - src/System.CommandLine/IdentifierSymbol.cs | 10 ++++------ src/System.CommandLine/Symbol.cs | 2 +- 3 files changed, 5 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 d42c624ad9..4d2cf91e3e 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 @@ -148,7 +148,6 @@ System.CommandLine public interface IConsole : System.CommandLine.IO.IStandardError, System.CommandLine.IO.IStandardIn, System.CommandLine.IO.IStandardOut public abstract class IdentifierSymbol : Symbol public System.Collections.Generic.IReadOnlyCollection Aliases { get; } - public System.String Name { get; set; } public System.Void AddAlias(System.String alias) public System.Boolean HasAlias(System.String alias) public class LocalizationResources diff --git a/src/System.CommandLine/IdentifierSymbol.cs b/src/System.CommandLine/IdentifierSymbol.cs index 95e16e92fe..57db0f38d9 100644 --- a/src/System.CommandLine/IdentifierSymbol.cs +++ b/src/System.CommandLine/IdentifierSymbol.cs @@ -12,7 +12,6 @@ namespace System.CommandLine public abstract class IdentifierSymbol : Symbol { private protected readonly HashSet _aliases = new(StringComparer.Ordinal); - private string? _specifiedName; /// /// Initializes a new instance of the class. @@ -42,19 +41,18 @@ protected IdentifierSymbol(string name, string? description = null) /// public override string Name { - get => _specifiedName ??= DefaultName; set { - if (_specifiedName is null || !string.Equals(_specifiedName, value, StringComparison.Ordinal)) + if (_name is null || !string.Equals(_name, value, StringComparison.Ordinal)) { AddAlias(value); - if (_specifiedName is { }) + if (_name is { }) { - RemoveAlias(_specifiedName); + RemoveAlias(_name); } - _specifiedName = value; + _name = value; } } } diff --git a/src/System.CommandLine/Symbol.cs b/src/System.CommandLine/Symbol.cs index df408ba2f3..0538fdfe07 100644 --- a/src/System.CommandLine/Symbol.cs +++ b/src/System.CommandLine/Symbol.cs @@ -11,7 +11,7 @@ namespace System.CommandLine /// public abstract class Symbol { - private string? _name; + private protected string? _name; private ParentNode? _firstParent; private protected Symbol() From 492f4f02f63e672436783404c1880b2d6d734bd2 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 12 Dec 2022 21:56:15 +0100 Subject: [PATCH 2/7] remove Option._name, just use Symbol._name --- src/System.CommandLine/Option.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/System.CommandLine/Option.cs b/src/System.CommandLine/Option.cs index 50b667dfe0..27f09489a7 100644 --- a/src/System.CommandLine/Option.cs +++ b/src/System.CommandLine/Option.cs @@ -15,7 +15,6 @@ namespace System.CommandLine /// public abstract class Option : IdentifierSymbol, IValueDescriptor { - private string? _name; private List>? _validators; private protected Option(string name, string? description) : base(description) @@ -156,7 +155,7 @@ internal virtual bool IsGreedy object? IValueDescriptor.GetDefaultValue() => Argument.GetDefaultValue(); - private protected override string DefaultName => _name ??= GetLongestAlias(); + private protected override string DefaultName => GetLongestAlias(); private string GetLongestAlias() { From 9ffe9ffef787c82a5a295958f49eb2a4db576bb0 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 12 Dec 2022 22:15:54 +0100 Subject: [PATCH 3/7] don't use first alias in ArgumentConversionResult, use the longest one move GetLongestAlias to IdentifierSymbol and re-use where possible --- .../Binding/ArgumentConversionResult.cs | 2 +- src/System.CommandLine/IdentifierSymbol.cs | 14 ++++++++++++++ src/System.CommandLine/LocalizationResources.cs | 2 +- src/System.CommandLine/Option.cs | 15 +-------------- 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/System.CommandLine/Binding/ArgumentConversionResult.cs b/src/System.CommandLine/Binding/ArgumentConversionResult.cs index 9cc56e88f2..2e0ac7b25f 100644 --- a/src/System.CommandLine/Binding/ArgumentConversionResult.cs +++ b/src/System.CommandLine/Binding/ArgumentConversionResult.cs @@ -56,7 +56,7 @@ private static string FormatErrorMessage( if (argument.FirstParent?.Symbol is IdentifierSymbol identifierSymbol && argument.FirstParent.Next is null) { - var alias = identifierSymbol.Aliases.First(); + var alias = identifierSymbol.GetLongestAlias(removePrefix: false); switch (identifierSymbol) { diff --git a/src/System.CommandLine/IdentifierSymbol.cs b/src/System.CommandLine/IdentifierSymbol.cs index 57db0f38d9..2b8bb0e856 100644 --- a/src/System.CommandLine/IdentifierSymbol.cs +++ b/src/System.CommandLine/IdentifierSymbol.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.Parsing; using System.Diagnostics; namespace System.CommandLine @@ -80,6 +81,19 @@ public void AddAlias(string alias) /// if the alias has already been defined; otherwise . public bool HasAlias(string alias) => _aliases.Contains(alias); + internal string GetLongestAlias(bool removePrefix) + { + string max = ""; + foreach (string alias in _aliases) + { + if (alias.Length > max.Length) + { + max = alias; + } + } + return removePrefix ? max.RemovePrefix() : max; + } + [DebuggerStepThrough] private void ThrowIfAliasIsInvalid(string alias) { diff --git a/src/System.CommandLine/LocalizationResources.cs b/src/System.CommandLine/LocalizationResources.cs index 0107ce06fe..45cc33b8de 100644 --- a/src/System.CommandLine/LocalizationResources.cs +++ b/src/System.CommandLine/LocalizationResources.cs @@ -102,7 +102,7 @@ public virtual string RequiredCommandWasNotProvided() => /// Interpolates values into a localized string similar to Option '{0}' is required. /// public virtual string RequiredOptionWasNotProvided(Option option) => - GetResourceString(Properties.Resources.RequiredOptionWasNotProvided, option.Aliases.OrderByDescending(x => x.Length).First()); + GetResourceString(Properties.Resources.RequiredOptionWasNotProvided, option.GetLongestAlias(removePrefix: false)); /// /// Interpolates values into a localized string similar to Argument '{0}' not recognized. Must be one of:{1}. diff --git a/src/System.CommandLine/Option.cs b/src/System.CommandLine/Option.cs index 27f09489a7..7858c2c950 100644 --- a/src/System.CommandLine/Option.cs +++ b/src/System.CommandLine/Option.cs @@ -155,20 +155,7 @@ internal virtual bool IsGreedy object? IValueDescriptor.GetDefaultValue() => Argument.GetDefaultValue(); - private protected override string DefaultName => GetLongestAlias(); - - private string GetLongestAlias() - { - string max = ""; - foreach (string alias in _aliases) - { - if (alias.Length > max.Length) - { - max = alias; - } - } - return max.RemovePrefix(); - } + private protected override string DefaultName => GetLongestAlias(true); /// public override IEnumerable GetCompletions(CompletionContext context) From 0fa443ca9268f546792ff820402c48fbde6f1524 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 12 Dec 2022 22:28:07 +0100 Subject: [PATCH 4/7] move Option.HasAliasIgnoringPrefix to DragonFruit which is it's only caller --- ...ommandLine_api_is_not_changed.approved.txt | 1 - .../CommandLine.cs | 36 ++++++++++++++++++- src/System.CommandLine.Tests/OptionTests.cs | 17 --------- src/System.CommandLine/Option.cs | 20 ----------- .../Parsing/StringExtensions.cs | 2 +- 5 files changed, 36 insertions(+), 40 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 4d2cf91e3e..b989eb855b 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 @@ -195,7 +195,6 @@ System.CommandLine public System.Boolean IsRequired { get; set; } public System.Type ValueType { get; } public System.Collections.Generic.IEnumerable GetCompletions(System.CommandLine.Completions.CompletionContext context) - public System.Boolean HasAliasIgnoringPrefix(System.String alias) public ParseResult Parse(System.String commandLine) public ParseResult Parse(System.String[] args) public class Option : Option, IValueDescriptor, System.CommandLine.Binding.IValueDescriptor diff --git a/src/System.CommandLine.DragonFruit/CommandLine.cs b/src/System.CommandLine.DragonFruit/CommandLine.cs index e9c407aa87..582ace0701 100644 --- a/src/System.CommandLine.DragonFruit/CommandLine.cs +++ b/src/System.CommandLine.DragonFruit/CommandLine.cs @@ -197,7 +197,7 @@ public static CommandLineBuilder ConfigureHelpFromXmlComments( { var kebabCasedParameterName = parameterDescription.Key.ToKebabCase(); - var option = builder.Command.Options.FirstOrDefault(o => o.HasAliasIgnoringPrefix(kebabCasedParameterName)); + var option = builder.Command.Options.FirstOrDefault(o => HasAliasIgnoringPrefix(o, kebabCasedParameterName)); if (option != null) { @@ -300,5 +300,39 @@ private static string GetDefaultXmlDocsFileLocation(Assembly assembly) return string.Empty; } + + /// + /// Indicates whether a given alias exists on the option, regardless of its prefix. + /// + /// The alias, which can include a prefix. + /// if the alias exists; otherwise, . + private static bool HasAliasIgnoringPrefix(Option option, string alias) + { + ReadOnlySpan rawAlias = alias.AsSpan(GetPrefixLength(alias)); + + foreach (string existingAlias in option.Aliases) + { + if (MemoryExtensions.Equals(existingAlias.AsSpan(GetPrefixLength(existingAlias)), rawAlias, StringComparison.CurrentCulture)) + { + return true; + } + } + + return false; + + static int GetPrefixLength(string alias) + { + if (alias[0] == '-') + { + return alias.Length > 1 && alias[1] == '-' ? 2 : 1; + } + else if (alias[0] == '/') + { + return 1; + } + + return 0; + } + } } } diff --git a/src/System.CommandLine.Tests/OptionTests.cs b/src/System.CommandLine.Tests/OptionTests.cs index bcc8bd5990..56b4a7752c 100644 --- a/src/System.CommandLine.Tests/OptionTests.cs +++ b/src/System.CommandLine.Tests/OptionTests.cs @@ -64,7 +64,6 @@ public void A_prefixed_alias_can_be_added_to_an_option() option.AddAlias("-a"); - option.HasAliasIgnoringPrefix("a").Should().BeTrue(); option.HasAlias("-a").Should().BeTrue(); } @@ -84,14 +83,6 @@ public void HasAlias_accepts_prefixed_short_value() option.HasAlias("-o").Should().BeTrue(); } - [Fact] - public void HasAliasIgnorePrefix_accepts_unprefixed_short_value() - { - var option = new Option(new[] { "-o", "--option" }); - - option.HasAliasIgnoringPrefix("o").Should().BeTrue(); - } - [Fact] public void HasAlias_accepts_prefixed_long_value() { @@ -100,14 +91,6 @@ public void HasAlias_accepts_prefixed_long_value() option.HasAlias("--option").Should().BeTrue(); } - [Fact] - public void HasAliasIgnorePrefix_accepts_unprefixed_long_value() - { - var option = new Option(new[] { "-o", "--option" }); - - option.HasAliasIgnoringPrefix("option").Should().BeTrue(); - } - [Fact] public void It_is_not_necessary_to_specify_a_prefix_when_adding_an_option() { diff --git a/src/System.CommandLine/Option.cs b/src/System.CommandLine/Option.cs index 7858c2c950..a810e5e930 100644 --- a/src/System.CommandLine/Option.cs +++ b/src/System.CommandLine/Option.cs @@ -100,26 +100,6 @@ public override string Name internal bool HasValidators => _validators is not null && _validators.Count > 0; - /// - /// Indicates whether a given alias exists on the option, regardless of its prefix. - /// - /// The alias, which can include a prefix. - /// if the alias exists; otherwise, . - public bool HasAliasIgnoringPrefix(string alias) - { - ReadOnlySpan rawAlias = alias.AsSpan(alias.GetPrefixLength()); - - foreach (string existingAlias in _aliases) - { - if (MemoryExtensions.Equals(existingAlias.AsSpan(existingAlias.GetPrefixLength()), rawAlias, StringComparison.CurrentCulture)) - { - return true; - } - } - - return false; - } - /// /// Gets a value that indicates whether multiple argument tokens are allowed for each option identifier token. /// diff --git a/src/System.CommandLine/Parsing/StringExtensions.cs b/src/System.CommandLine/Parsing/StringExtensions.cs index 5de4fbcbe3..6fcf60328a 100644 --- a/src/System.CommandLine/Parsing/StringExtensions.cs +++ b/src/System.CommandLine/Parsing/StringExtensions.cs @@ -32,7 +32,7 @@ internal static string RemovePrefix(this string alias) : alias; } - internal static int GetPrefixLength(this string alias) + private static int GetPrefixLength(this string alias) { if (alias[0] == '-') { From 4ff392d7334fd13ddf2357006066cd6a313854c2 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 12 Dec 2022 22:28:54 +0100 Subject: [PATCH 5/7] make IdentifierSymbol._aliases private (not private protected) --- src/System.CommandLine/IdentifierSymbol.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.CommandLine/IdentifierSymbol.cs b/src/System.CommandLine/IdentifierSymbol.cs index 2b8bb0e856..668af4d47c 100644 --- a/src/System.CommandLine/IdentifierSymbol.cs +++ b/src/System.CommandLine/IdentifierSymbol.cs @@ -12,7 +12,7 @@ namespace System.CommandLine /// public abstract class IdentifierSymbol : Symbol { - private protected readonly HashSet _aliases = new(StringComparer.Ordinal); + private readonly HashSet _aliases = new(StringComparer.Ordinal); /// /// Initializes a new instance of the class. From b7936d0967469878f82101b089e904d138c58e13 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 13 Dec 2022 14:16:50 +0100 Subject: [PATCH 6/7] further simplification --- src/System.CommandLine/IdentifierSymbol.cs | 2 +- src/System.CommandLine/Option.cs | 19 +------------------ .../Parsing/SymbolResultExtensions.cs | 7 +------ 3 files changed, 3 insertions(+), 25 deletions(-) diff --git a/src/System.CommandLine/IdentifierSymbol.cs b/src/System.CommandLine/IdentifierSymbol.cs index 668af4d47c..b5820caedf 100644 --- a/src/System.CommandLine/IdentifierSymbol.cs +++ b/src/System.CommandLine/IdentifierSymbol.cs @@ -30,7 +30,7 @@ protected IdentifierSymbol(string? description = null) /// The description of the symbol, which is displayed in command line help. protected IdentifierSymbol(string name, string? description = null) { - Name = name; + Name = name ?? throw new ArgumentNullException(nameof(name)); Description = description; } diff --git a/src/System.CommandLine/Option.cs b/src/System.CommandLine/Option.cs index a810e5e930..1a9ad0f7c8 100644 --- a/src/System.CommandLine/Option.cs +++ b/src/System.CommandLine/Option.cs @@ -24,8 +24,6 @@ private protected Option(string name, string? description) : base(description) throw new ArgumentNullException(nameof(name)); } - _name = name.RemovePrefix(); - AddAlias(name); } @@ -81,21 +79,6 @@ public ArgumentArity Arity internal bool DisallowBinding { get; init; } - /// - public override string Name - { - set - { - if (!HasAlias(value)) - { - _name = null; - RemoveAlias(DefaultName); - } - - base.Name = value; - } - } - internal List> Validators => _validators ??= new(); internal bool HasValidators => _validators is not null && _validators.Count > 0; @@ -116,7 +99,7 @@ public override string Name public bool AllowMultipleArgumentsPerToken { get; set; } internal virtual bool IsGreedy - => Argument is not null && Argument.Arity.MinimumNumberOfValues > 0 && Argument.ValueType != typeof(bool); + => Argument.Arity.MinimumNumberOfValues > 0 && Argument.ValueType != typeof(bool); /// /// Indicates whether the option is required when its parent command is invoked. diff --git a/src/System.CommandLine/Parsing/SymbolResultExtensions.cs b/src/System.CommandLine/Parsing/SymbolResultExtensions.cs index a5e76d7fdf..e447f37ada 100644 --- a/src/System.CommandLine/Parsing/SymbolResultExtensions.cs +++ b/src/System.CommandLine/Parsing/SymbolResultExtensions.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.Linq; namespace System.CommandLine.Parsing { @@ -31,11 +30,7 @@ internal static Token Token(this SymbolResult symbolResult) Token CreateImplicitToken(Option option) { - var optionName = option.Name; - - var defaultAlias = option.Aliases.First(alias => alias.RemovePrefix() == optionName); - - return new Token(defaultAlias, TokenType.Option, option, Parsing.Token.ImplicitPosition); + return new Token(option.GetLongestAlias(removePrefix: false), TokenType.Option, option, Parsing.Token.ImplicitPosition); } } } From 16792fabbe1499f5a96a66a3ff50958e1a051e5b Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 14 Dec 2022 08:13:57 +0100 Subject: [PATCH 7/7] 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/IdentifierSymbol.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.CommandLine/IdentifierSymbol.cs b/src/System.CommandLine/IdentifierSymbol.cs index b5820caedf..d52b09d2d9 100644 --- a/src/System.CommandLine/IdentifierSymbol.cs +++ b/src/System.CommandLine/IdentifierSymbol.cs @@ -48,7 +48,7 @@ public override string Name { AddAlias(value); - if (_name is { }) + if (_name != null) { RemoveAlias(_name); }