Skip to content

Commit 993ea1e

Browse files
KathleenDollardKathleenDollard
committed
Changed to TryGet... in ValueSources for consistency
1 parent b92fbfa commit 993ea1e

File tree

10 files changed

+200
-129
lines changed

10 files changed

+200
-129
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ public void Values_above_calculated_upper_bound_report_error()
143143
public void Values_below_relative_lower_bound_report_error()
144144
{
145145
var otherOption = new CliOption<int>("-a");
146-
var option = GetOptionWithRangeBounds(ValueSource<int>.Create(otherOption, o => (true, (int)o + 1)), 50);
146+
var option = GetOptionWithRangeBounds(ValueSource.Create(otherOption, o => (true, (int)o + 1)), 50);
147147
var command = new CliCommand("cmd") { option, otherOption };
148148

149149
var pipelineResult = ExecutedPipelineResultForCommand(command, "--intOpt 0 -a 0");

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

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

44
using FluentAssertions;
5-
using Microsoft.VisualBasic.FileIO;
65
using System.CommandLine.Parsing;
7-
using System.CommandLine.ValueConditions;
86
using System.CommandLine.ValueSources;
97
using Xunit;
108

@@ -28,51 +26,55 @@ public void SimpleValueSource_with_set_value_retrieved()
2826
{
2927
var valueSource = new SimpleValueSource<int>(42);
3028

31-
(bool success, int value) = valueSource.GetTypedValue(EmptyPipelineResult());
32-
33-
success.Should()
34-
.BeTrue();
35-
value.Should()
36-
.Be(42);
29+
if (valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value))
30+
{
31+
value.Should()
32+
.Be(42);
33+
return;
34+
}
35+
Assert.Fail("Typed value not retrieved");
3736
}
3837

3938
[Fact]
4039
public void SimpleValueSource_with_converted_value_retrieved()
4140
{
4241
ValueSource<int> valueSource = 42;
4342

44-
(bool success, int value) = valueSource.GetTypedValue(EmptyPipelineResult());
45-
46-
success.Should()
47-
.BeTrue();
48-
value.Should()
49-
.Be(42);
43+
if (valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value))
44+
{
45+
value.Should()
46+
.Be(42);
47+
return;
48+
}
49+
Assert.Fail("Typed value not retrieved");
5050
}
5151

5252
[Fact]
5353
public void SimpleValueSource_created_via_extension_value_retrieved()
5454
{
5555
var valueSource = ValueSource.Create(42);
5656

57-
(bool success, int value) = valueSource.GetTypedValue(EmptyPipelineResult());
58-
59-
success.Should()
60-
.BeTrue();
61-
value.Should()
62-
.Be(42);
57+
if (valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value))
58+
{
59+
value.Should()
60+
.Be(42);
61+
return;
62+
}
63+
Assert.Fail("Typed value not retrieved");
6364
}
6465

6566
[Fact]
6667
public void CalculatedValueSource_produces_value()
6768
{
6869
var valueSource = new CalculatedValueSource<int>(() => (true, 42));
6970

70-
(bool success, int value) = valueSource.GetTypedValue(EmptyPipelineResult());
71-
72-
success.Should()
73-
.BeTrue();
74-
value.Should()
75-
.Be(42);
71+
if (valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value))
72+
{
73+
value.Should()
74+
.Be(42);
75+
return;
76+
}
77+
Assert.Fail("Typed value not retrieved");
7678
}
7779

7880
[Fact]
@@ -82,24 +84,27 @@ public void CalculatedValueSource_implicitly_converted_produces_value()
8284
// ValueSource<int> valueSource2 = (() => 42);
8385
ValueSource<int> valueSource = (ValueSource<int>)(() => (true, 42)); ;
8486

85-
(bool success, int value) = valueSource.GetTypedValue(EmptyPipelineResult());
86-
87-
success.Should()
88-
.BeTrue();
89-
value.Should()
90-
.Be(42);
87+
if (valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value))
88+
{
89+
value.Should()
90+
.Be(42);
91+
return;
92+
}
93+
Assert.Fail("Typed value not retrieved");
9194
}
9295

9396
[Fact]
9497
public void CalculatedValueSource_from_extension_produces_value()
9598
{
9699
var valueSource = ValueSource.Create(() => (true, 42));
97-
(bool success, int value) = valueSource.GetTypedValue(EmptyPipelineResult());
98100

99-
success.Should()
100-
.BeTrue();
101-
value.Should()
102-
.Be(42);
101+
if (valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value))
102+
{
103+
value.Should()
104+
.Be(42);
105+
return;
106+
}
107+
Assert.Fail("Typed value not retrieved");
103108
}
104109

105110
[Fact]
@@ -108,12 +113,14 @@ public void RelativeToSymbolValueSource_produces_value_that_was_set()
108113
var option = new CliOption<int>("-a");
109114
var valueSource = new RelativeToSymbolValueSource<int>(option);
110115

111-
(bool success, int value) = valueSource.GetTypedValue(EmptyPipelineResult("-a 42", option));
112-
113-
success.Should()
114-
.BeTrue();
115-
value.Should()
116-
.Be(42);
116+
if (valueSource.TryGetTypedValue(EmptyPipelineResult("-a 42", option), out var value))
117+
{
118+
value.Should()
119+
.Be(42);
120+
return;
121+
}
122+
Assert.Fail("Typed value not retrieved");
123+
117124
}
118125

119126
[Fact]
@@ -122,12 +129,13 @@ public void RelativeToSymbolValueSource_implicitly_converted_produces_value_that
122129
var option = new CliOption<int>("-a");
123130
ValueSource<int> valueSource = option;
124131

125-
(bool success, int value) = valueSource.GetTypedValue(EmptyPipelineResult("-a 42", option));
126-
127-
success.Should()
128-
.BeTrue();
129-
value.Should()
130-
.Be(42);
132+
if (valueSource.TryGetTypedValue(EmptyPipelineResult("-a 42", option), out var value))
133+
{
134+
value.Should()
135+
.Be(42);
136+
return;
137+
}
138+
Assert.Fail("Typed value not retrieved");
131139
}
132140

133141
[Fact]
@@ -136,30 +144,32 @@ public void RelativeToSymbolValueSource_from_extension_produces_value_that_was_s
136144
var option = new CliOption<int>("-a");
137145
var valueSource = new RelativeToSymbolValueSource<int>(option);
138146

139-
(bool success, int value) = valueSource.GetTypedValue(EmptyPipelineResult("-a 42", option));
140-
141-
success.Should()
142-
.BeTrue();
143-
value.Should()
144-
.Be(42);
147+
if (valueSource.TryGetTypedValue(EmptyPipelineResult("-a 42", option), out var value))
148+
{
149+
value.Should()
150+
.Be(42);
151+
return;
152+
}
153+
Assert.Fail("Typed value not retrieved");
145154
}
146155

147156
[Fact]
148157
public void RelativeToEnvironmentVariableValueSource_produces_value_that_was_set()
149158
{
150159
var envName = "SYSTEM_COMMANDLINE_TESTING";
151160
var valueSource = new RelativeToEnvironmentVariableValueSource<int>(envName);
152-
161+
153162
Environment.SetEnvironmentVariable(envName, "42");
154-
(bool success, int value) = valueSource.GetTypedValue(EmptyPipelineResult(""));
163+
if (valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value))
164+
{
165+
value.Should()
166+
.Be(42);
167+
return;
168+
}
155169
Environment.SetEnvironmentVariable(envName, null);
156-
157-
success.Should()
158-
.BeTrue();
159-
value.Should()
160-
.Be(42);
170+
Assert.Fail("Typed value not retrieved");
161171
}
162-
172+
163173

164174
[Fact]
165175
public void RelativeToEnvironmentVariableValueSource_from_extension_produces_value_that_was_set()
@@ -168,12 +178,13 @@ public void RelativeToEnvironmentVariableValueSource_from_extension_produces_val
168178
var valueSource = ValueSource.CreateFromEnvironmentVariable<int>(envName);
169179

170180
Environment.SetEnvironmentVariable(envName, "42");
171-
(bool success, int value) = valueSource.GetTypedValue(EmptyPipelineResult(""));
181+
if (valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value))
182+
{
183+
value.Should()
184+
.Be(42);
185+
return;
186+
}
172187
Environment.SetEnvironmentVariable(envName, null);
173-
174-
success.Should()
175-
.BeTrue();
176-
value.Should()
177-
.Be(42);
188+
Assert.Fail("Typed value not retrieved");
178189
}
179190
}

‎src/System.CommandLine.Subsystems/ValueConditions/Range.cs

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -33,22 +33,19 @@ public void Validate(object? value,
3333
if (comparableValue is null) return; // nothing to do
3434

3535
// TODO: Replace the strings we are comparing with a diagnostic ID when we update ParseError
36-
if (LowerBound is not null)
36+
if (LowerBound is not null
37+
&& LowerBound.TryGetTypedValue(validationContext.PipelineResult, out var lowerValue)
38+
&& comparableValue.CompareTo(lowerValue) < 0)
3739
{
38-
var lower = LowerBound.GetTypedValue(validationContext.PipelineResult);
39-
if (lower.success && comparableValue.CompareTo(lower.value) < 0)
40-
{
41-
validationContext.PipelineResult.AddError(new ParseError($"The value for '{valueSymbol.Name}' is below the lower bound of {LowerBound}"));
42-
}
40+
validationContext.PipelineResult.AddError(new ParseError($"The value for '{valueSymbol.Name}' is below the lower bound of {LowerBound}"));
4341
}
4442

45-
if (UpperBound is not null)
43+
44+
if (UpperBound is not null
45+
&& UpperBound.TryGetTypedValue(validationContext.PipelineResult, out var upperValue)
46+
&& comparableValue.CompareTo(upperValue) > 0)
4647
{
47-
var upper = UpperBound.GetTypedValue(validationContext.PipelineResult);
48-
if (upper.success && comparableValue.CompareTo(upper.value) > 0)
49-
{
50-
validationContext.PipelineResult.AddError(new ParseError($"The value for '{valueSymbol.Name}' is above the upper bound of {UpperBound}"));
51-
}
48+
validationContext.PipelineResult.AddError(new ParseError($"The value for '{valueSymbol.Name}' is above the upper bound of {UpperBound}"));
5249
}
5350
}
5451

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,12 +51,11 @@ private bool TryGetValue<T>(CliSymbol symbol, out T? value)
5151
}
5252
if (valueSymbol.TryGetDefaultValueSource(out ValueSource? defaultValueSource))
5353
{
54-
if (defaultValueSource is not ValueSource<T> typedValueSource)
54+
if (defaultValueSource is not ValueSource<T> typedDefaultValueSource)
5555
{
5656
throw new InvalidOperationException("Unexpected ValueSource type");
5757
}
58-
(var success, var defaultValue) = typedValueSource.GetTypedValue(pipelineResult);
59-
if (success)
58+
if( typedDefaultValueSource.TryGetTypedValue(pipelineResult, out T? defaultValue))
6059
{
6160
return UseValue(valueSymbol, defaultValue);
6261
}

‎src/System.CommandLine.Subsystems/ValueSources/AggregateValueSource.cs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,25 +21,25 @@ public AggregateValueSource(ValueSource<T> firstSource,
2121

2222
public bool PrecedenceAsEntered { get; set; }
2323

24-
public override (bool success, T? value) GetTypedValue(PipelineResult pipelineResult)
25-
=> ValueFromSources(pipelineResult);
26-
27-
private (bool success, T? value) ValueFromSources(PipelineResult pipelineResult)
24+
public override bool TryGetTypedValue(PipelineResult pipelineResult, out T? value)
2825
{
2926
var orderedSources = PrecedenceAsEntered
3027
? valueSources
3128
: [.. valueSources.OrderBy(GetPrecedence)];
3229
foreach (var source in orderedSources)
3330
{
34-
(var success, var value) = source.GetTypedValue(pipelineResult);
35-
if (success)
31+
if (source.TryGetTypedValue(pipelineResult, out var newValue))
3632
{
37-
return (true, value);
33+
value = newValue;
34+
return true;
3835
}
3936
}
40-
return (false, default);
37+
value = default;
38+
return false;
39+
4140
}
4241

42+
// TODO: Discuss precedence vs order entered for aggregates
4343
internal static int GetPrecedence(ValueSource<T> source)
4444
{
4545
return source switch

‎src/System.CommandLine.Subsystems/ValueSources/CalculatedValueSource.cs

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,16 @@ public class CalculatedValueSource<T>(Func<(bool success, T? value)> calculation
99
{
1010
public override string? Description { get; } = description;
1111

12-
public override (bool success, T? value) GetTypedValue(PipelineResult pipelineResult)
13-
=> calculation();
12+
public override bool TryGetTypedValue(PipelineResult pipelineResult, out T? value)
13+
{
14+
(bool success, T? newValue) = calculation();
15+
if (success)
16+
{
17+
value = newValue;
18+
return true;
19+
}
20+
value = default;
21+
return false;
22+
}
1423
}
1524

‎src/System.CommandLine.Subsystems/ValueSources/RelativeToEnvironmentVariableValueSource.cs

Lines changed: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,25 +10,41 @@ public class RelativeToEnvironmentVariableValueSource<T>(string environmentVaria
1010
{
1111
public override string? Description { get; } = description;
1212

13-
public override (bool success, T? value) GetTypedValue(PipelineResult pipelineResult)
13+
public override bool TryGetTypedValue(PipelineResult pipelineResult, out T? value)
1414
{
1515
string? stringValue = Environment.GetEnvironmentVariable(environmentVariableName);
1616

1717
if (stringValue is null)
1818
{
19-
// This feels wrong. It isn't saying "Hey, you asked for a value that was not there"
20-
return default;
19+
value = default;
20+
return false;
2121
}
2222

23-
// TODO: What is the best way to do this?
24-
T value = default(T) switch
23+
// TODO: Unify this with System.CommandLine.ArgumentConverter conversions, which will require changes to that code.
24+
// This will provide consistency, including support for nullable value types, and custom type conversions
25+
try
2526
{
26-
int i => (T)(object)Convert.ToInt32(stringValue),
27-
_ => throw new NotImplementedException("Looking for a non-dumb way to do this")
28-
};
29-
return calculation is null
30-
? (true, value)
31-
: calculation(Environment.GetEnvironmentVariable(environmentVariableName));
27+
if (calculation is not null)
28+
{
29+
(var success, var calcValue) = calculation(stringValue);
30+
if (success)
31+
{
32+
value = calcValue;
33+
return true;
34+
}
35+
value = default;
36+
return false;
37+
}
38+
var newValue = Convert.ChangeType(stringValue, typeof(T));
39+
value = (T?)newValue;
40+
return true;
41+
}
42+
catch
43+
{
44+
// TODO: This probably represents a failure converting from string, so in user's world to fix. How do we report this?
45+
value = default;
46+
return false;
47+
}
3248
}
3349
}
3450

0 commit comments

Comments
 (0)