Skip to content

Commit 6094c81

Browse files
KathleenDollardKathleenDollard
committed
Respond to group PR Review
1 parent d8b18ed commit 6094c81

File tree

14 files changed

+127
-103
lines changed

14 files changed

+127
-103
lines changed

‎src/System.CommandLine.Subsystems.Tests/ValueSubsystemTests.cs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ namespace System.CommandLine.Subsystems.Tests;
1111
public class ValueSubsystemTests
1212
{
1313
[Fact]
14-
public void Value_is_always_activated()
14+
public void ValueSubsystem_is_activated_by_default()
1515
{
1616
CliRootCommand rootCommand = [
1717
new CliCommand("x")
@@ -65,7 +65,6 @@ public void ValueSubsystem_returns_default_value_when_no_value_is_entered()
6565
const int expected = 43;
6666
var input = $"";
6767

68-
var parseResult = pipeline.Parse(configuration, input); // assigned for debugging
6968
pipeline.Execute(configuration, input, consoleHack);
7069

7170
pipeline.Value.GetValue<int>(option).Should().Be(expected);
@@ -84,7 +83,7 @@ public void ValueSubsystem_returns_calculated_default_value_when_no_value_is_ent
8483
var x = 42;
8584
pipeline.Value.DefaultValueCalculation.Set(option, () => x + 2);
8685
const int expected = 44;
87-
var input = $"";
86+
var input = "";
8887

8988
var parseResult = pipeline.Parse(configuration, input); // assigned for debugging
9089
pipeline.Execute(configuration, input, consoleHack);

‎src/System.CommandLine.Subsystems/Pipeline.cs

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
using System.CommandLine.Directives;
55
using System.CommandLine.Parsing;
66
using System.CommandLine.Subsystems;
7-
using System.Reflection.PortableExecutable;
87

98
namespace System.CommandLine;
109

@@ -30,7 +29,7 @@ public static Pipeline Create(HelpSubsystem? help = null,
3029
Value = value ?? new ValueSubsystem()
3130
};
3231

33-
public static Pipeline CreateEmpty()
32+
public static Pipeline CreateEmpty()
3433
=> new();
3534

3635
private Pipeline() { }
@@ -83,9 +82,9 @@ protected virtual void InitializeSubsystems(InitializationContext context)
8382
{
8483
foreach (var subsystem in Subsystems)
8584
{
86-
if ( subsystem is not null)
85+
if (subsystem is not null)
8786
{
88-
subsystem.Initialize(context);
87+
subsystem.Initialize(context);
8988
}
9089
}
9190
}

‎src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationAccessor.cs

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,6 @@ namespace System.CommandLine.Subsystems.Annotations;
88
/// <summary>
99
/// Allows associating an annotation with a <see cref="CliSymbol"/>. The annotation will be stored by the accessor's owner <see cref="CliSubsystem"/>.
1010
/// </summary>
11-
/// <remarks>
12-
/// The annotation will be stored by the accessor's owner <see cref="CliSubsystem"/>.
13-
/// </summary>
14-
/// <typeparam name="TValue">The type of value to be stored</typeparam>
15-
/// <param name="owner">The subsystem that this annotation store data for.</param>
16-
/// <param name="id">The identifier for this annotation, since subsystems may have multiple annotations.</param>
1711
public struct AnnotationAccessor<TValue>(CliSubsystem owner, AnnotationId<TValue> id)
1812
{
1913
/// <summary>

‎src/System.CommandLine.Subsystems/Subsystems/Annotations/ValueAnnotationAccessor.cs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,12 @@ private readonly bool TryGetInternal<TSymbolValue>(CliSymbol symbol, [NotNullWhe
5454
{
5555
if (owner.TryGetAnnotation(symbol, id, out var storedValue))
5656
{
57-
value = (TSymbolValue)storedValue;
58-
return true;
57+
if (storedValue is TSymbolValue symbolValue)
58+
{
59+
value = symbolValue;
60+
return true;
61+
}
62+
throw new ArgumentException("The requested type is incorrect.", nameof(symbol));
5963
}
6064
value = default;
6165
return false;

‎src/System.CommandLine.Subsystems/Subsystems/Annotations/ValueFuncAnnotationAccessor.cs

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -6,51 +6,46 @@
66
namespace System.CommandLine.Subsystems.Annotations;
77

88
/// <summary>
9-
/// Associates an annotation with a <see cref="CliSymbol"/>. The symbol must be an option or argument and the delegate must return a value of the same type as the symbol./>.
9+
/// Associates an annotation with a <see cref="CliSymbol"/>. The symbol must be an option or argument and the delegate must return a value of the same type as the symbol.
1010
/// </summary>
1111
/// <remarks>
1212
/// The annotation will be stored by the accessor's owner <see cref="CliSubsystem"/>.
1313
/// </remarks>
1414
/// <typeparam name="TValue">The type of value to be stored</typeparam>
1515
/// <param name="owner">The subsystem that this annotation store data for.</param>
1616
/// <param name="id">The identifier for this annotation, since subsystems may have multiple annotations.</param>
17+
// TODO: If we keep this approach, consider making this class more general purpose or replacing use with AnnotationAccessior
1718
public struct ValueFuncAnnotationAccessor<TValue>(CliSubsystem owner, AnnotationId<Func<TValue>> id)
1819
{
1920
/// <inheritdoc cref="AnnotationAccessor{TValue}.Id"/>>
2021
public AnnotationId<Func<TValue>> Id { get; }
2122

2223
/// <inheritdoc cref="AnnotationAccessor{TValue}.Set"/>>
23-
public readonly void Set<TSymbolValue>(CliOption<TSymbolValue> symbol, Func<TValue> value)
24-
where TSymbolValue : TValue
24+
public readonly void Set(CliOption symbol, Func<TValue> value)
2525
=> owner.SetAnnotation(symbol, id, value);
2626

2727
/// <inheritdoc cref="AnnotationAccessor{TValue}.Set"/>>
28-
public readonly void Set<TSymbolValue>(CliArgument<TSymbolValue> symbol, Func<TValue> value)
29-
where TSymbolValue : TValue
28+
public readonly void Set(CliArgument symbol, Func<TValue> value)
3029
=> owner.SetAnnotation(symbol, id, value);
3130

3231
// TODO: Consider whether we need a version that takes a CliSymbol (ValueSymbol)
3332
/// <inheritdoc cref="AnnotationAccessor{TValue}.Get"/>>
34-
public readonly bool TryGet<TSymbolValue>(CliOption<TSymbolValue> symbol, [NotNullWhen(true)] out Func<TValue>? value)
35-
where TSymbolValue : TValue
36-
=> TryGetInternal<TSymbolValue>(symbol, out value);
33+
public readonly bool TryGet(CliOption symbol, [NotNullWhen(true)] out Func<TValue>? value)
34+
=> TryGetInternal(symbol, out value);
3735

3836
/// <inheritdoc cref="AnnotationAccessor{TValue}.Get"/>>
39-
public readonly bool TryGet<TSymbolValue>(CliArgument<TSymbolValue> symbol, [NotNullWhen(true)] out Func<TValue>? value)
40-
where TSymbolValue : TValue
41-
=> TryGetInternal<TSymbolValue>(symbol, out value);
37+
public readonly bool TryGet(CliArgument symbol, [NotNullWhen(true)] out Func<TValue>? value)
38+
=> TryGetInternal(symbol, out value);
4239

4340
/// <inheritdoc cref="AnnotationAccessor{TValue}.Get"/>>
4441
/// <remarks>
4542
/// This overload will throw if the stored value cannot be converted to the type.
4643
/// </remarks>
4744
/// <exception cref="InvalidCastException"/>
48-
public readonly bool TryGet<TSymbolValue>(CliSymbol symbol, [NotNullWhen(true)] out Func<TValue>? value)
49-
where TSymbolValue : TValue
50-
=> TryGetInternal<TSymbolValue>(symbol, out value);
45+
public readonly bool TryGet(CliSymbol symbol, [NotNullWhen(true)] out Func<TValue>? value)
46+
=> TryGetInternal(symbol, out value);
5147

52-
private readonly bool TryGetInternal<TSymbolValue>(CliSymbol symbol, [NotNullWhen(true)] out Func<TValue>? value)
53-
where TSymbolValue : TValue
48+
private readonly bool TryGetInternal(CliSymbol symbol, [NotNullWhen(true)] out Func<TValue>? value)
5449
{
5550
if (owner.TryGetAnnotation(symbol, id, out Func<TValue>? storedValue))
5651
{

‎src/System.CommandLine.Subsystems/Subsystems/CliSubsystem.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ protected CliSubsystem(string name, SubsystemKind subsystemKind, IAnnotationProv
2929
public SubsystemKind SubsystemKind { get; }
3030

3131
private DefaultAnnotationProvider? _defaultProvider;
32-
private readonly IAnnotationProvider? _annotationProvider;
32+
private readonly IAnnotationProvider? _annotationProvider;
3333

3434
/// <summary>
3535
/// Attempt to retrieve the value for the symbol and annotation ID from the annotation provider.

‎src/System.CommandLine.Subsystems/ValueSubsystem.cs

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
namespace System.CommandLine;
88

9-
public class ValueSubsystem(IAnnotationProvider? annotationProvider = null)
9+
public class ValueSubsystem(IAnnotationProvider? annotationProvider = null)
1010
: CliSubsystem(ValueAnnotations.Prefix, SubsystemKind.Value, annotationProvider)
1111
{
1212
private Dictionary<CliSymbol, object?> cachedValues = [];
@@ -22,7 +22,7 @@ public ValueAnnotationAccessor<object?> DefaultValue
2222
/// Provides access to Get and Set methods for default value calculations for symbols
2323
/// </summary>
2424
public ValueFuncAnnotationAccessor<object?> DefaultValueCalculation
25-
=> new (this, ValueAnnotations.DefaultValueCalculation);
25+
=> new(this, ValueAnnotations.DefaultValueCalculation);
2626

2727
// It is possible that another subsystems GetIsActivated method will access a value.
2828
// If this is called from a GetIsActivated method of a subsystem in the early termination group,
@@ -48,14 +48,17 @@ protected internal override CliExit Execute(PipelineContext pipelineContext)
4848
}
4949

5050
private void SetValue<T>(CliSymbol symbol, object? value)
51-
=> cachedValues.Add(symbol, value);
51+
{
52+
cachedValues[symbol] = value;
53+
}
54+
5255
private bool TryGetValue<T>(CliSymbol symbol, out T? value)
5356
{
5457
if (cachedValues.TryGetValue(symbol, out var objectValue))
5558
{
5659
value = objectValue is null
5760
? default
58-
:(T)objectValue;
61+
: (T)objectValue;
5962
return true;
6063
}
6164
value = default;
@@ -68,25 +71,34 @@ private bool TryGetValue<T>(CliSymbol symbol, out T? value)
6871
=> GetValueInternal<T>(argument);
6972

7073
private T? GetValueInternal<T>(CliSymbol? symbol)
71-
=> symbol switch
74+
{
75+
return symbol switch
7276
{
7377
not null when TryGetValue<T>(symbol, out var value)
7478
=> value, // It has already been retrieved at least once
75-
CliArgument argument when parseResult?.GetValueResult(argument) is { } valueResult // GetValue not used because it would always return a value
79+
CliArgument argument when parseResult?.GetValueResult(argument) is { } valueResult // GetValue not used because it would always return a value
7680
=> UseValue(symbol, valueResult.GetValue<T>()), // Value was supplied during parsing,
77-
CliOption option when parseResult?.GetValueResult(option) is {} valueResult // GetValue not used because it would always return a value
81+
CliOption option when parseResult?.GetValueResult(option) is { } valueResult // GetValue not used because it would always return a value
7882
=> UseValue(symbol, valueResult.GetValue<T>()), // Value was supplied during parsing
7983
// Value was not supplied during parsing, determine default now
80-
not null when DefaultValueCalculation.TryGet<T>(symbol, out var defaultValueCalculation)
81-
=> UseValue(symbol, CalculatedDefault<T>(symbol, defaultValueCalculation)),
82-
not null when DefaultValue.TryGet<T>(symbol, out var explicitValue)
83-
=> UseValue<T>(symbol, (T)explicitValue),
84+
// configuration values go here in precedence
8485
//not null when GetDefaultFromEnvironmentVariable<T>(symbol, out var envName)
8586
// => UseValue(symbol, GetEnvByName(envName)),
87+
not null when DefaultValueCalculation.TryGet(symbol, out var defaultValueCalculation)
88+
=> UseValue(symbol, CalculatedDefault<T>(symbol, defaultValueCalculation)),
89+
not null when DefaultValue.TryGet<T>(symbol, out var explicitValue)
90+
=> UseValue<T>(symbol, (T)explicitValue),
8691
null => throw new ArgumentNullException(nameof(symbol)),
8792
_ => UseValue(symbol, default(T))
8893
};
8994

95+
TValue? UseValue<TValue>(CliSymbol symbol, TValue? value)
96+
{
97+
SetValue<TValue>(symbol, value);
98+
return value;
99+
}
100+
}
101+
90102
private static T? CalculatedDefault<T>(CliSymbol symbol, Func<object?> defaultValueCalculation)
91103
{
92104
var objectValue = defaultValueCalculation();
@@ -95,10 +107,4 @@ not null when DefaultValue.TryGet<T>(symbol, out var explicitValue)
95107
: (T)objectValue;
96108
return value;
97109
}
98-
99-
private T? UseValue<T>(CliSymbol symbol, T? value)
100-
{
101-
SetValue<T>(symbol, value);
102-
return value;
103-
}
104110
}

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

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -17,42 +17,43 @@ public void Symbol_found_by_name()
1717
var option2 = new CliOption<string>("--opt2");
1818

1919
var rootCommand = new CliRootCommand
20-
{
21-
option1,
22-
option2
23-
};
20+
{
21+
option1,
22+
option2
23+
};
2424

2525
var parseResult = CliParser.Parse(rootCommand, "--opt1 Kirk");
2626

2727
var symbol1 = parseResult.GetSymbolByName("--opt1");
2828
var symbol2 = parseResult.GetSymbolByName("--opt2");
2929
using (new AssertionScope())
3030
{
31-
symbol1.Should().Be(option1);
32-
symbol2.Should().Be(option2);
31+
symbol1.Should().Be(option1, "because option1 should be found for --opt1" );
32+
symbol2.Should().Be(option2, "because option2 should be found for --opt2");
3333
}
3434
}
3535

3636
[Fact]
3737
public void Nearest_symbol_found_when_multiple()
3838
{
39-
var option1 = new CliOption<string>("--opt1", "-1");
40-
var option2 = new CliOption<string>("--opt1", "-2");
39+
// both options have the same name as that is the point of the test
40+
var optionA = new CliOption<string>("--opt1", "-1");
41+
var optionB = new CliOption<string>("--opt1", "-2");
4142

4243
var command = new CliCommand("subcommand")
43-
{
44-
option2
45-
};
44+
{
45+
optionB
46+
};
4647

4748
var rootCommand = new CliRootCommand
48-
{
49-
command,
50-
option1
51-
};
49+
{
50+
command,
51+
optionA
52+
};
5253

53-
var parseResult = CliParser.Parse(rootCommand, "subcommand --opt2 Spock");
54+
var parseResult = CliParser.Parse(rootCommand, "subcommand");
5455

5556
var symbol = parseResult.GetSymbolByName("--opt1");
56-
symbol.Should().Be(option2);
57+
symbol.Should().Be(optionB, "because it is closer to the leaf/executing command");
5758
}
58-
}
59+
}

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1890,10 +1890,8 @@ public void Locations_correct_for_collection()
18901890

18911891
var parseResult = CliParser.Parse(rootCommand, "subcommand --opt1 Kirk Spock Uhura");
18921892

1893-
var commandResult = parseResult.CommandResult;
1894-
parseResult.GetValue(option1);
1895-
var result1 = parseResult.GetValueResult(option1);
1896-
result1.Locations.Should().BeEquivalentTo([expectedLocation1, expectedLocation2, expectedLocation3]);
1893+
var result = parseResult.GetValueResult(option1);
1894+
result.Locations.Should().BeEquivalentTo([expectedLocation1, expectedLocation2, expectedLocation3]);
18971895
}
18981896

18991897
[Fact]

‎src/System.CommandLine/ParseResult.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ internal ParseResult(
8787
symbolLookupByName ??= new SymbolLookupByName(this);
8888
return symbolLookupByName.TryGetSymbol(name, out var symbol, valuesOnly: valuesOnly)
8989
? symbol
90-
: throw new ArgumentException($"No symbol result found with name \"{name}\".");
90+
: throw new ArgumentException($"No symbol result found with name \"{name}\".", nameof(name));
9191
}
9292

9393
// TODO: check that constructing empty ParseResult directly is correct
@@ -206,7 +206,7 @@ CommandLineText is null
206206
/// Gets the result, if any, for the specified argument.
207207
/// </summary>
208208
/// <param name="argument">The argument for which to find a result.</param>
209-
/// <returns>A result for the specified argument, or <see langword="default"/> if it was not entered by the user.</returns>
209+
/// <returns>A result for the specified argument, or <see langword="null"/> if it was not entered by the user.</returns>
210210
public ValueResult? GetValueResult(CliArgument argument)
211211
=> GetValueResultInternal(argument);
212212

0 commit comments

Comments
 (0)