Skip to content

Commit b2ac225

Browse files
KathleenDollardKathleenDollard
committed
Changes listed - updated Command/ValueResult, Value subsystem, and cleanup
System.CommandLine: - CommandValueResult, made a tree - ValueResult - added text for later display, like `main`-ish - Created new `SymbolByName` - Cleanup ValueSubsystem - Renamed DefaultValue and CalculatedDefaultValue - Created cache for values - Limited GetValue/Try... to Arg and Option Test - Added ParseResultValueTest and tests in support of other work
1 parent dfa55d0 commit b2ac225

File tree

10 files changed

+239
-98
lines changed

10 files changed

+239
-98
lines changed

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

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,15 @@ public class ValueSubsystemTests
1313
[Fact]
1414
public void Value_is_always_activated()
1515
{
16-
CliRootCommand rootCommand = [new CliCommand("x")];
16+
CliRootCommand rootCommand = [
17+
new CliCommand("x")
18+
{
19+
new CliOption<string>("--opt1")
20+
}];
1721
var configuration = new CliConfiguration(rootCommand);
1822
var subsystem = new ValueSubsystem();
19-
string[] args = ["x"];
23+
var input = "x --opt1 Kirk";
24+
var args = CliParser.SplitCommandLine(input).ToList();
2025

2126
Subsystem.Initialize(subsystem, configuration, args);
2227
var parseResult = CliParser.Parse(rootCommand, args[0], configuration);
@@ -25,19 +30,28 @@ public void Value_is_always_activated()
2530
isActive.Should().BeTrue();
2631
}
2732

28-
[Theory]
29-
[ClassData(typeof(TestData.Value))]
30-
public void Diagram_is_activated_only_when_requested(string input, bool expectedIsActive)
33+
[Fact]
34+
public void ValueSubsystem_returns_values_that_are_entered()
3135
{
32-
CliRootCommand rootCommand = [new CliCommand("x")];
36+
CliRootCommand rootCommand = [
37+
new CliCommand("x")
38+
{
39+
new CliOption<int>("--intValue"),
40+
new CliOption<string>("--stringValue"),
41+
new CliOption<bool>("--boolValue")
42+
}];
3343
var configuration = new CliConfiguration(rootCommand);
34-
var subsystem = new DiagramSubsystem();
35-
var args = CliParser.SplitCommandLine(input).ToList().AsReadOnly();
44+
var subsystem = new ValueSubsystem();
45+
const int expected1 = 42;
46+
const string expected2 = "43";
47+
var input = $"x --intValue {expected1} --stringValue \"{expected2}\" --boolValue";
48+
var args = CliParser.SplitCommandLine(input).ToList();
3649

3750
Subsystem.Initialize(subsystem, configuration, args);
3851
var parseResult = CliParser.Parse(rootCommand, input, configuration);
39-
var isActive = Subsystem.GetIsActivated(subsystem, parseResult);
4052

41-
isActive.Should().Be(expectedIsActive);
53+
parseResult.GetValue<int>("--intValue").Should().Be(expected1);
54+
parseResult.GetValue<string>("--stringValue").Should().Be(expected2);
55+
parseResult.GetValue<bool>("--boolValue").Should().Be(true);
4256
}
4357
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ public static class ValueAnnotations
1212
{
1313
public static string Prefix { get; } = nameof(SubsystemKind.Value);
1414

15-
public static AnnotationId<object?> ExplicitDefault { get; } = new(Prefix, nameof(ExplicitDefault));
16-
public static AnnotationId<Func<object?>?> DefaultCalculation { get; } = new(Prefix, nameof(DefaultCalculation));
15+
public static AnnotationId<object?> DefaultValue { get; } = new(Prefix, nameof(DefaultValue));
16+
public static AnnotationId<Func<object?>?> DefaultValueCalculation { get; } = new(Prefix, nameof(DefaultValueCalculation));
1717
public static AnnotationId<object?> Value { get; } = new(Prefix, nameof(Value));
1818
}

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

Lines changed: 30 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -12,19 +12,19 @@ public class ValueSubsystem : CliSubsystem
1212
// @mhutch: Is the TryGet on the sparse dictionaries how we should handle a case where the annotations will be sparse to support lazy? If so, should we have another method on
1313
// the annotation wrapper, or an alternative struct when there a TryGet makes sense? This API needs review, maybe next Tuesday.
1414
private PipelineContext? pipelineContext = null;
15+
private Dictionary<CliSymbol, object?> cachedValues = new();
1516

1617
public ValueSubsystem(IAnnotationProvider? annotationProvider = null)
1718
: base(ValueAnnotations.Prefix, SubsystemKind.Version, annotationProvider)
1819
{ }
1920

20-
//TODO: ExplicitDefault might have a valid null value, and thus try pattern. DefaultCalculation is only null when not present. Consider whether to use the same pattern (try on DefaultCalculation, even though it is not needed)
21-
internal void SetExplicitDefault(CliSymbol symbol, object? defaultValue)
22-
=> SetAnnotation(symbol, ValueAnnotations.ExplicitDefault, defaultValue);
23-
internal object? GetExplicitDefault(CliSymbol symbol)
24-
=> TryGetAnnotation(symbol, ValueAnnotations.ExplicitDefault, out var defaultValue)
21+
internal void SetDefaultValue(CliSymbol symbol, object? defaultValue)
22+
=> SetAnnotation(symbol, ValueAnnotations.DefaultValue, defaultValue);
23+
internal object? GetDefaultValue(CliSymbol symbol)
24+
=> TryGetAnnotation(symbol, ValueAnnotations.DefaultValue, out var defaultValue)
2525
? defaultValue
2626
: "";
27-
internal bool TryGetExplicitDefault<T>(CliSymbol symbol, out T? defaultValue)
27+
internal bool TryGetDefaultValue<T>(CliSymbol symbol, out T? defaultValue)
2828
{
2929
if (TryGetAnnotation(symbol, ValueAnnotations.Value, out var objectValue))
3030
{
@@ -34,17 +34,17 @@ internal bool TryGetExplicitDefault<T>(CliSymbol symbol, out T? defaultValue)
3434
defaultValue = default;
3535
return false;
3636
}
37-
public AnnotationAccessor<object?> ExplicitDefault
38-
=> new(this, ValueAnnotations.ExplicitDefault);
37+
public AnnotationAccessor<object?> DefaultValue
38+
=> new(this, ValueAnnotations.DefaultValue);
3939

40-
internal void SetDefaultCalculation(CliSymbol symbol, Func<object?> factory)
41-
=> SetAnnotation(symbol, ValueAnnotations.DefaultCalculation, factory);
42-
internal Func<object?>? GetDefaultCalculation(CliSymbol symbol)
43-
=> TryGetAnnotation<Func<object?>?>(symbol, ValueAnnotations.DefaultCalculation, out var value)
40+
internal void SetDefaultValueCalculation(CliSymbol symbol, Func<object?> factory)
41+
=> SetAnnotation(symbol, ValueAnnotations.DefaultValueCalculation, factory);
42+
internal Func<object?>? GetDefaultValueCalculation(CliSymbol symbol)
43+
=> TryGetAnnotation<Func<object?>?>(symbol, ValueAnnotations.DefaultValueCalculation, out var value)
4444
? value
4545
: null;
46-
public AnnotationAccessor<Func<object?>?> DefaultCalculation
47-
=> new(this, ValueAnnotations.DefaultCalculation);
46+
public AnnotationAccessor<Func<object?>?> DefaultValueCalculation
47+
=> new(this, ValueAnnotations.DefaultValueCalculation);
4848

4949
protected internal override bool GetIsActivated(ParseResult? parseResult)
5050
=> true;
@@ -55,82 +55,48 @@ protected internal override CliExit Execute(PipelineContext pipelineContext)
5555
return CliExit.NotRun(pipelineContext.ParseResult);
5656
}
5757

58-
// TODO: Consider using a simple dictionary instead of the annotation (@mhutch) because with is not useful here
58+
59+
// TODO: Do it! Consider using a simple dictionary instead of the annotation (@mhutch) because with is not useful here
5960
private void SetValue<T>(CliSymbol symbol, object? value)
60-
=> SetAnnotation(symbol, ValueAnnotations.Value, value);
61-
// TODO: Consider a way to disallow CliCommand here, as it creates a pit of failure.
61+
=> cachedValues.Add(symbol, value);
6262
private bool TryGetValue<T>(CliSymbol symbol, out T? value)
6363
{
64-
if (TryGetAnnotation(symbol, ValueAnnotations.Value, out var objectValue))
64+
if (cachedValues.TryGetValue(symbol, out var objectValue))
6565
{
66-
value = (T)objectValue;
66+
value = objectValue is null
67+
? default
68+
:(T)objectValue;
6769
return true;
6870
}
6971
value = default;
7072
return false;
7173
}
72-
// TODO: Is fluent style meaningful for Value?
73-
//public AnnotationAccessor<object?> Value
74-
// => new(this, ValueAnnotations.Value);
7574

7675
public T? GetValue<T>(CliOption option)
7776
=> GetValueInternal<T>(option);
7877
public T? GetValue<T>(CliArgument argument)
7978
=> GetValueInternal<T>(argument);
8079

81-
// TODO: @mhutch: I find this more readable than the if conditional version below. There will be at least two more blocks. Look good?
82-
private T? GetValueInternal<T>(CliSymbol symbol)
80+
private T? GetValueInternal<T>(CliSymbol? symbol)
8381
=> symbol switch
8482
{
85-
{ } when TryGetValue<T>(symbol, out var value)
83+
not null when TryGetValue<T>(symbol, out var value)
8684
=> value, // It has already been retrieved at least once
87-
{ } when pipelineContext?.ParseResult?.GetValueResult(symbol) is ValueResult valueResult
85+
CliArgument argument when pipelineContext?.ParseResult?.GetValueResult(argument) is ValueResult valueResult // GetValue would always return a value
86+
=> UseValue(symbol, valueResult.GetValue<T>()), // Value was supplied during parsing,
87+
CliOption option when pipelineContext?.ParseResult?.GetValueResult(option) is ValueResult valueResult // GetValue would always return a value
8888
=> UseValue(symbol, valueResult.GetValue<T>()), // Value was supplied during parsing
8989
// Value was not supplied during parsing, determine default now
90-
{ } when GetDefaultCalculation(symbol) is { } defaultValueCalculation
90+
not null when DefaultValueCalculation.TryGet(symbol, out var defaultValueCalculation)
9191
=> UseValue(symbol, CalculatedDefault<T>(symbol, defaultValueCalculation)),
92-
{ } when TryGetExplicitDefault<T>(symbol, out var explicitValue)
92+
not null when TryGetDefaultValue<T>(symbol, out var explicitValue)
9393
=> UseValue(symbol, explicitValue),
94+
//not null when GetDefaultFromEnvironmentVariable<T>(symbol, out var envName)
95+
// => UseValue(symbol, GetEnvByName(envName)),
9496
null => throw new ArgumentNullException(nameof(symbol)),
9597
_ => UseValue(symbol, default(T))
9698
};
9799

98-
//// The following is temporarily included for showing why the above weird code is cleaner
99-
//public T? GetValue2<T>(CliSymbol symbol)
100-
//{
101-
// if (TryGetValue<T>(symbol, out var value))
102-
// {
103-
// // It has already been retrieved at least once
104-
// return value;
105-
// }
106-
// if (pipelineContext?.ParseResult?.GetValueResult(symbol) is ValueResult valueResult)
107-
// {
108-
// // Value was supplied during parsing
109-
// return UseValue(symbol, valueResult.GetValue<T>());
110-
// }
111-
// // Value was not supplied during parsing, determine default now
112-
// if (GetDefaultCalculation(symbol) is { } defaultValueCalculation)
113-
// {
114-
// return UseValue(symbol, CalculatedDefault(symbol, defaultValueCalculation));
115-
// }
116-
// if (TryGetExplicitDefault<T>(symbol, out var explicitValue))
117-
// {
118-
// return UseValue(symbol, value);
119-
// }
120-
// value = default;
121-
// SetValue<T>(symbol, value);
122-
// return value;
123-
124-
// static T? CalculatedDefault(CliSymbol symbol, Func<object?> defaultValueCalculation)
125-
// {
126-
// var objectValue = defaultValueCalculation();
127-
// var value = objectValue is null
128-
// ? default
129-
// : (T)objectValue;
130-
// return value;
131-
// }
132-
//}
133-
134100
private static T? CalculatedDefault<T>(CliSymbol symbol, Func<object?> defaultValueCalculation)
135101
{
136102
var objectValue = defaultValueCalculation();

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

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
// Copyright (c) .NET Foundation and contributors. All rights reserved.
2+
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
3+
4+
using System.CommandLine.Parsing;
5+
using System.Linq;
6+
using FluentAssertions;
7+
using Xunit;
8+
9+
namespace System.CommandLine.Tests;
10+
11+
public class ParseResultValueTests
12+
{
13+
[Fact]
14+
public void Symbol_found_by_name()
15+
{
16+
var option1 = new CliOption<string>("--opt1");
17+
var option2 = new CliOption<string>("--opt2");
18+
19+
var rootCommand = new CliRootCommand
20+
{
21+
option1,
22+
option2
23+
};
24+
25+
var parseResult = CliParser.Parse(rootCommand, "--opt1 Kirk");
26+
27+
var symbol1 = parseResult.GetSymbolByName("--opt1");
28+
var symbol2 = parseResult.GetSymbolByName("--opt2");
29+
symbol1.Should().Be(option1);
30+
symbol2.Should().Be(option2);
31+
}
32+
33+
[Fact]
34+
public void Nearest_symbol_found_when_multiple()
35+
{
36+
var option1 = new CliOption<string>("--opt1", "-1");
37+
var option2 = new CliOption<string>("--opt1", "-2");
38+
39+
var command = new CliCommand("subcommand")
40+
{
41+
option2
42+
};
43+
44+
var rootCommand = new CliRootCommand
45+
{
46+
command,
47+
option1
48+
};
49+
50+
var parseResult = CliParser.Parse(rootCommand, "subcommand --opt2 Spock");
51+
52+
var symbol = parseResult.GetSymbolByName("--opt1");
53+
symbol.Should().Be(option2);
54+
}
55+
}

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

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1256,12 +1256,12 @@ public void Single_option_arguments_that_match_option_aliases_are_parsed_correct
12561256
}
12571257

12581258
[Theory]
1259-
[InlineData("-x -y")]
1260-
[InlineData("-x true -y")]
1261-
[InlineData("-x:true -y")]
1262-
[InlineData("-x=true -y")]
1259+
[InlineData("-x -y")] //
1260+
[InlineData("-x true -y")] //
1261+
[InlineData("-x:true -y")] //
1262+
[InlineData("-x=true -y")] //
12631263
[InlineData("-x -y true")]
1264-
[InlineData("-x true -y true")]
1264+
[InlineData("-x true -y true")] //
12651265
[InlineData("-x:true -y:true")]
12661266
[InlineData("-x=true -y:true")]
12671267
public void Boolean_options_are_not_greedy(string commandLine)
@@ -1874,6 +1874,28 @@ public void Location_offset_correct_when_colon_or_equal_used()
18741874
result2.Locations.Single().Should().Be(expectedLocation2);
18751875
}
18761876

1877+
[Fact]
1878+
public void Locations_correct_for_collection()
1879+
{
1880+
var option1 = new CliOption<string[]>("--opt1");
1881+
option1.AllowMultipleArgumentsPerToken = true;
1882+
var rootCommand = new CliRootCommand
1883+
{
1884+
option1
1885+
};
1886+
var expectedOuterLocation = new Location("testhost", Location.User, -1, null);
1887+
var expectedLocation1 = new Location("Kirk", Location.User, 2, expectedOuterLocation);
1888+
var expectedLocation2 = new Location("Spock", Location.User, 3, expectedOuterLocation);
1889+
var expectedLocation3 = new Location("Uhura", Location.User, 4, expectedOuterLocation);
1890+
1891+
var parseResult = CliParser.Parse(rootCommand, "subcommand --opt1 Kirk Spock Uhura");
1892+
1893+
var commandResult = parseResult.CommandResult;
1894+
parseResult.GetValue(option1);
1895+
var result1 = parseResult.GetValueResult(option1);
1896+
result1.Locations.Should().BeEquivalentTo([expectedLocation1, expectedLocation2, expectedLocation3]);
1897+
}
1898+
18771899
[Fact]
18781900
public void ParseResult_contains_argument_ValueResults()
18791901
{

‎src/System.CommandLine.Tests/System.CommandLine.Tests.csproj

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,10 @@
2020
<!--
2121
<Compile Include="..\System.CommandLine\Properties\Resources.Designer.cs" Link="Properties\Resources.Designer.cs" />
2222
-->
23+
<Compile Include="ParseResultValueTests.cs" />
2324
<Compile Include="TokenizerTests.cs" />
2425
<Compile Include="ParserTests.cs" />
25-
<Compile Include="Utility\**\*.cs" Exclude="Utility\ParseResultExtensions.cs"/>
26+
<Compile Include="Utility\**\*.cs" Exclude="Utility\ParseResultExtensions.cs" />
2627
</ItemGroup>
2728

2829
<ItemGroup>

0 commit comments

Comments
 (0)