Skip to content

Commit 18027bd

Browse files
adamsitnikjozkee
adamsitnik
and
jozkee
authored
name and aliases cleanup (#1990)
* remove IdentifierSymbol._specifiedName, just use Symbol._name * remove Option._name, just use Symbol._name * don't use first alias in ArgumentConversionResult, use the longest one move GetLongestAlias to IdentifierSymbol and re-use where possible * move Option.HasAliasIgnoringPrefix to DragonFruit which is it's only caller * make IdentifierSymbol._aliases private (not private protected) * further simplification * Apply suggestions from code review Co-authored-by: David Cantú <[email protected]>
1 parent f449c2d commit 18027bd

File tree

10 files changed

+62
-91
lines changed

10 files changed

+62
-91
lines changed

‎src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,6 @@ System.CommandLine
148148
public interface IConsole : System.CommandLine.IO.IStandardError, System.CommandLine.IO.IStandardIn, System.CommandLine.IO.IStandardOut
149149
public abstract class IdentifierSymbol : Symbol
150150
public System.Collections.Generic.IReadOnlyCollection<System.String> Aliases { get; }
151-
public System.String Name { get; set; }
152151
public System.Void AddAlias(System.String alias)
153152
public System.Boolean HasAlias(System.String alias)
154153
public class LocalizationResources
@@ -196,7 +195,6 @@ System.CommandLine
196195
public System.Boolean IsRequired { get; set; }
197196
public System.Type ValueType { get; }
198197
public System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem> GetCompletions(System.CommandLine.Completions.CompletionContext context)
199-
public System.Boolean HasAliasIgnoringPrefix(System.String alias)
200198
public ParseResult Parse(System.String commandLine)
201199
public ParseResult Parse(System.String[] args)
202200
public class Option<T> : Option, IValueDescriptor<T>, System.CommandLine.Binding.IValueDescriptor

‎src/System.CommandLine.DragonFruit/CommandLine.cs

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,7 @@ public static CommandLineBuilder ConfigureHelpFromXmlComments(
197197
{
198198
var kebabCasedParameterName = parameterDescription.Key.ToKebabCase();
199199

200-
var option = builder.Command.Options.FirstOrDefault(o => o.HasAliasIgnoringPrefix(kebabCasedParameterName));
200+
var option = builder.Command.Options.FirstOrDefault(o => HasAliasIgnoringPrefix(o, kebabCasedParameterName));
201201

202202
if (option != null)
203203
{
@@ -300,5 +300,39 @@ private static string GetDefaultXmlDocsFileLocation(Assembly assembly)
300300

301301
return string.Empty;
302302
}
303+
304+
/// <summary>
305+
/// Indicates whether a given alias exists on the option, regardless of its prefix.
306+
/// </summary>
307+
/// <param name="alias">The alias, which can include a prefix.</param>
308+
/// <returns><see langword="true"/> if the alias exists; otherwise, <see langword="false"/>.</returns>
309+
private static bool HasAliasIgnoringPrefix(Option option, string alias)
310+
{
311+
ReadOnlySpan<char> rawAlias = alias.AsSpan(GetPrefixLength(alias));
312+
313+
foreach (string existingAlias in option.Aliases)
314+
{
315+
if (MemoryExtensions.Equals(existingAlias.AsSpan(GetPrefixLength(existingAlias)), rawAlias, StringComparison.CurrentCulture))
316+
{
317+
return true;
318+
}
319+
}
320+
321+
return false;
322+
323+
static int GetPrefixLength(string alias)
324+
{
325+
if (alias[0] == '-')
326+
{
327+
return alias.Length > 1 && alias[1] == '-' ? 2 : 1;
328+
}
329+
else if (alias[0] == '/')
330+
{
331+
return 1;
332+
}
333+
334+
return 0;
335+
}
336+
}
303337
}
304338
}

‎src/System.CommandLine.Tests/OptionTests.cs

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@ public void A_prefixed_alias_can_be_added_to_an_option()
6464

6565
option.AddAlias("-a");
6666

67-
option.HasAliasIgnoringPrefix("a").Should().BeTrue();
6867
option.HasAlias("-a").Should().BeTrue();
6968
}
7069

@@ -84,14 +83,6 @@ public void HasAlias_accepts_prefixed_short_value()
8483
option.HasAlias("-o").Should().BeTrue();
8584
}
8685

87-
[Fact]
88-
public void HasAliasIgnorePrefix_accepts_unprefixed_short_value()
89-
{
90-
var option = new Option<string>(new[] { "-o", "--option" });
91-
92-
option.HasAliasIgnoringPrefix("o").Should().BeTrue();
93-
}
94-
9586
[Fact]
9687
public void HasAlias_accepts_prefixed_long_value()
9788
{
@@ -100,14 +91,6 @@ public void HasAlias_accepts_prefixed_long_value()
10091
option.HasAlias("--option").Should().BeTrue();
10192
}
10293

103-
[Fact]
104-
public void HasAliasIgnorePrefix_accepts_unprefixed_long_value()
105-
{
106-
var option = new Option<string>(new[] { "-o", "--option" });
107-
108-
option.HasAliasIgnoringPrefix("option").Should().BeTrue();
109-
}
110-
11194
[Fact]
11295
public void It_is_not_necessary_to_specify_a_prefix_when_adding_an_option()
11396
{

‎src/System.CommandLine/Binding/ArgumentConversionResult.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ private static string FormatErrorMessage(
5656
if (argument.FirstParent?.Symbol is IdentifierSymbol identifierSymbol &&
5757
argument.FirstParent.Next is null)
5858
{
59-
var alias = identifierSymbol.Aliases.First();
59+
var alias = identifierSymbol.GetLongestAlias(removePrefix: false);
6060

6161
switch (identifierSymbol)
6262
{

‎src/System.CommandLine/IdentifierSymbol.cs

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
33

44
using System.Collections.Generic;
5+
using System.CommandLine.Parsing;
56
using System.Diagnostics;
67

78
namespace System.CommandLine
@@ -11,8 +12,7 @@ namespace System.CommandLine
1112
/// </summary>
1213
public abstract class IdentifierSymbol : Symbol
1314
{
14-
private protected readonly HashSet<string> _aliases = new(StringComparer.Ordinal);
15-
private string? _specifiedName;
15+
private readonly HashSet<string> _aliases = new(StringComparer.Ordinal);
1616

1717
/// <summary>
1818
/// Initializes a new instance of the <see cref="IdentifierSymbol"/> class.
@@ -30,7 +30,7 @@ protected IdentifierSymbol(string? description = null)
3030
/// <param name="description">The description of the symbol, which is displayed in command line help.</param>
3131
protected IdentifierSymbol(string name, string? description = null)
3232
{
33-
Name = name;
33+
Name = name ?? throw new ArgumentNullException(nameof(name));
3434
Description = description;
3535
}
3636

@@ -42,19 +42,18 @@ protected IdentifierSymbol(string name, string? description = null)
4242
/// <inheritdoc/>
4343
public override string Name
4444
{
45-
get => _specifiedName ??= DefaultName;
4645
set
4746
{
48-
if (_specifiedName is null || !string.Equals(_specifiedName, value, StringComparison.Ordinal))
47+
if (_name is null || !string.Equals(_name, value, StringComparison.Ordinal))
4948
{
5049
AddAlias(value);
5150

52-
if (_specifiedName is { })
51+
if (_name != null)
5352
{
54-
RemoveAlias(_specifiedName);
53+
RemoveAlias(_name);
5554
}
5655

57-
_specifiedName = value;
56+
_name = value;
5857
}
5958
}
6059
}
@@ -82,6 +81,19 @@ public void AddAlias(string alias)
8281
/// <returns><see langword="true" /> if the alias has already been defined; otherwise <see langword="false" />.</returns>
8382
public bool HasAlias(string alias) => _aliases.Contains(alias);
8483

84+
internal string GetLongestAlias(bool removePrefix)
85+
{
86+
string max = "";
87+
foreach (string alias in _aliases)
88+
{
89+
if (alias.Length > max.Length)
90+
{
91+
max = alias;
92+
}
93+
}
94+
return removePrefix ? max.RemovePrefix() : max;
95+
}
96+
8597
[DebuggerStepThrough]
8698
private void ThrowIfAliasIsInvalid(string alias)
8799
{

‎src/System.CommandLine/LocalizationResources.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ public virtual string RequiredCommandWasNotProvided() =>
102102
/// Interpolates values into a localized string similar to Option '{0}' is required.
103103
/// </summary>
104104
public virtual string RequiredOptionWasNotProvided(Option option) =>
105-
GetResourceString(Properties.Resources.RequiredOptionWasNotProvided, option.Aliases.OrderByDescending(x => x.Length).First());
105+
GetResourceString(Properties.Resources.RequiredOptionWasNotProvided, option.GetLongestAlias(removePrefix: false));
106106

107107
/// <summary>
108108
/// Interpolates values into a localized string similar to Argument &apos;{0}&apos; not recognized. Must be one of:{1}.

‎src/System.CommandLine/Option.cs

Lines changed: 2 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ namespace System.CommandLine
1515
/// <seealso cref="IdentifierSymbol" />
1616
public abstract class Option : IdentifierSymbol, IValueDescriptor
1717
{
18-
private string? _name;
1918
private List<Action<OptionResult>>? _validators;
2019

2120
private protected Option(string name, string? description) : base(description)
@@ -25,8 +24,6 @@ private protected Option(string name, string? description) : base(description)
2524
throw new ArgumentNullException(nameof(name));
2625
}
2726

28-
_name = name.RemovePrefix();
29-
3027
AddAlias(name);
3128
}
3229

@@ -82,45 +79,10 @@ public ArgumentArity Arity
8279

8380
internal bool DisallowBinding { get; init; }
8481

85-
/// <inheritdoc />
86-
public override string Name
87-
{
88-
set
89-
{
90-
if (!HasAlias(value))
91-
{
92-
_name = null;
93-
RemoveAlias(DefaultName);
94-
}
95-
96-
base.Name = value;
97-
}
98-
}
99-
10082
internal List<Action<OptionResult>> Validators => _validators ??= new();
10183

10284
internal bool HasValidators => _validators is not null && _validators.Count > 0;
10385

104-
/// <summary>
105-
/// Indicates whether a given alias exists on the option, regardless of its prefix.
106-
/// </summary>
107-
/// <param name="alias">The alias, which can include a prefix.</param>
108-
/// <returns><see langword="true"/> if the alias exists; otherwise, <see langword="false"/>.</returns>
109-
public bool HasAliasIgnoringPrefix(string alias)
110-
{
111-
ReadOnlySpan<char> rawAlias = alias.AsSpan(alias.GetPrefixLength());
112-
113-
foreach (string existingAlias in _aliases)
114-
{
115-
if (MemoryExtensions.Equals(existingAlias.AsSpan(existingAlias.GetPrefixLength()), rawAlias, StringComparison.CurrentCulture))
116-
{
117-
return true;
118-
}
119-
}
120-
121-
return false;
122-
}
123-
12486
/// <summary>
12587
/// Gets a value that indicates whether multiple argument tokens are allowed for each option identifier token.
12688
/// </summary>
@@ -137,7 +99,7 @@ public bool HasAliasIgnoringPrefix(string alias)
13799
public bool AllowMultipleArgumentsPerToken { get; set; }
138100

139101
internal virtual bool IsGreedy
140-
=> Argument is not null && Argument.Arity.MinimumNumberOfValues > 0 && Argument.ValueType != typeof(bool);
102+
=> Argument.Arity.MinimumNumberOfValues > 0 && Argument.ValueType != typeof(bool);
141103

142104
/// <summary>
143105
/// Indicates whether the option is required when its parent command is invoked.
@@ -156,20 +118,7 @@ internal virtual bool IsGreedy
156118

157119
object? IValueDescriptor.GetDefaultValue() => Argument.GetDefaultValue();
158120

159-
private protected override string DefaultName => _name ??= GetLongestAlias();
160-
161-
private string GetLongestAlias()
162-
{
163-
string max = "";
164-
foreach (string alias in _aliases)
165-
{
166-
if (alias.Length > max.Length)
167-
{
168-
max = alias;
169-
}
170-
}
171-
return max.RemovePrefix();
172-
}
121+
private protected override string DefaultName => GetLongestAlias(true);
173122

174123
/// <inheritdoc />
175124
public override IEnumerable<CompletionItem> GetCompletions(CompletionContext context)

‎src/System.CommandLine/Parsing/StringExtensions.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ internal static string RemovePrefix(this string alias)
3232
: alias;
3333
}
3434

35-
internal static int GetPrefixLength(this string alias)
35+
private static int GetPrefixLength(this string alias)
3636
{
3737
if (alias[0] == '-')
3838
{

‎src/System.CommandLine/Parsing/SymbolResultExtensions.cs

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
33

44
using System.Collections.Generic;
5-
using System.Linq;
65

76
namespace System.CommandLine.Parsing
87
{
@@ -31,11 +30,7 @@ internal static Token Token(this SymbolResult symbolResult)
3130

3231
Token CreateImplicitToken(Option option)
3332
{
34-
var optionName = option.Name;
35-
36-
var defaultAlias = option.Aliases.First(alias => alias.RemovePrefix() == optionName);
37-
38-
return new Token(defaultAlias, TokenType.Option, option, Parsing.Token.ImplicitPosition);
33+
return new Token(option.GetLongestAlias(removePrefix: false), TokenType.Option, option, Parsing.Token.ImplicitPosition);
3934
}
4035
}
4136
}

‎src/System.CommandLine/Symbol.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ namespace System.CommandLine
1111
/// </summary>
1212
public abstract class Symbol
1313
{
14-
private string? _name;
14+
private protected string? _name;
1515
private ParentNode? _firstParent;
1616

1717
private protected Symbol()

0 commit comments

Comments
 (0)