Skip to content

Commit dfa55d0

Browse files
KathleenDollardKathleenDollard
committed
Work on ValueSubsystem/added ValueSubsystemTests
Worked on gathering ParseResult and improving Get and TryGet methods * Moved GetIsActivated and Execute to avoid splitting GetValue/TryGetValue * Updated the default value selection pipeline * Began writing tests
1 parent 917b291 commit dfa55d0

File tree

4 files changed

+115
-58
lines changed

4 files changed

+115
-58
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
-->
3333
<Compile Include="AlternateSubsystems.cs" />
3434
<Compile Include="Constants.cs" />
35+
<Compile Include="ValueSubsystemTests.cs" />
3536
<Compile Include="ResponseSubsystemTests.cs" />
3637
<Compile Include="DirectiveSubsystemTests.cs" />
3738
<Compile Include="DiagramSubsystemTests.cs" />

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,4 +79,18 @@ internal class Directive : IEnumerable<object[]>
7979

8080
IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
8181
}
82+
83+
internal class Value : IEnumerable<object[]>
84+
{
85+
private readonly List<object[]> _data =
86+
[
87+
["--intValue", 42],
88+
["--stringValue", "43"],
89+
["--boolValue", true]
90+
];
91+
92+
public IEnumerator<object[]> GetEnumerator() => _data.GetEnumerator();
93+
94+
IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
95+
}
8296
}

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

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
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 FluentAssertions;
5+
using System.CommandLine.Directives;
6+
using System.CommandLine.Parsing;
7+
using Xunit;
8+
9+
namespace System.CommandLine.Subsystems.Tests;
10+
11+
public class ValueSubsystemTests
12+
{
13+
[Fact]
14+
public void Value_is_always_activated()
15+
{
16+
CliRootCommand rootCommand = [new CliCommand("x")];
17+
var configuration = new CliConfiguration(rootCommand);
18+
var subsystem = new ValueSubsystem();
19+
string[] args = ["x"];
20+
21+
Subsystem.Initialize(subsystem, configuration, args);
22+
var parseResult = CliParser.Parse(rootCommand, args[0], configuration);
23+
var isActive = Subsystem.GetIsActivated(subsystem, parseResult);
24+
25+
isActive.Should().BeTrue();
26+
}
27+
28+
[Theory]
29+
[ClassData(typeof(TestData.Value))]
30+
public void Diagram_is_activated_only_when_requested(string input, bool expectedIsActive)
31+
{
32+
CliRootCommand rootCommand = [new CliCommand("x")];
33+
var configuration = new CliConfiguration(rootCommand);
34+
var subsystem = new DiagramSubsystem();
35+
var args = CliParser.SplitCommandLine(input).ToList().AsReadOnly();
36+
37+
Subsystem.Initialize(subsystem, configuration, args);
38+
var parseResult = CliParser.Parse(rootCommand, input, configuration);
39+
var isActive = Subsystem.GetIsActivated(subsystem, parseResult);
40+
41+
isActive.Should().Be(expectedIsActive);
42+
}
43+
}

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

Lines changed: 57 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ public ValueSubsystem(IAnnotationProvider? annotationProvider = null)
1717
: base(ValueAnnotations.Prefix, SubsystemKind.Version, annotationProvider)
1818
{ }
1919

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)
2021
internal void SetExplicitDefault(CliSymbol symbol, object? defaultValue)
2122
=> SetAnnotation(symbol, ValueAnnotations.ExplicitDefault, defaultValue);
2223
internal object? GetExplicitDefault(CliSymbol symbol)
@@ -45,16 +46,19 @@ internal void SetDefaultCalculation(CliSymbol symbol, Func<object?> factory)
4546
public AnnotationAccessor<Func<object?>?> DefaultCalculation
4647
=> new(this, ValueAnnotations.DefaultCalculation);
4748

48-
private void SetValue(CliSymbol symbol, object? value)
49+
protected internal override bool GetIsActivated(ParseResult? parseResult)
50+
=> true;
51+
52+
protected internal override CliExit Execute(PipelineContext pipelineContext)
53+
{
54+
this.pipelineContext = pipelineContext;
55+
return CliExit.NotRun(pipelineContext.ParseResult);
56+
}
57+
58+
// TODO: Consider using a simple dictionary instead of the annotation (@mhutch) because with is not useful here
59+
private void SetValue<T>(CliSymbol symbol, object? value)
4960
=> SetAnnotation(symbol, ValueAnnotations.Value, value);
50-
// TODO: Consider putting the logic in the generic version here
51-
// TODO: Consider using a simple dictionary instead of the annotation (@mhutch)
52-
// TODO: GetValue should call TryGetValue, not another call to TryGetAnnotation.
53-
// TODO: Should we provide an untyped value?
54-
private object? GetValue(CliSymbol symbol)
55-
=> TryGetAnnotation<object?>(symbol, ValueAnnotations.Value, out var value)
56-
? value
57-
: null;
61+
// TODO: Consider a way to disallow CliCommand here, as it creates a pit of failure.
5862
private bool TryGetValue<T>(CliSymbol symbol, out T? value)
5963
{
6064
if (TryGetAnnotation(symbol, ValueAnnotations.Value, out var objectValue))
@@ -69,17 +73,13 @@ private bool TryGetValue<T>(CliSymbol symbol, out T? value)
6973
//public AnnotationAccessor<object?> Value
7074
// => new(this, ValueAnnotations.Value);
7175

72-
protected internal override bool GetIsActivated(ParseResult? parseResult)
73-
=> true;
76+
public T? GetValue<T>(CliOption option)
77+
=> GetValueInternal<T>(option);
78+
public T? GetValue<T>(CliArgument argument)
79+
=> GetValueInternal<T>(argument);
7480

75-
protected internal override CliExit Execute(PipelineContext pipelineContext)
76-
{
77-
this.pipelineContext = pipelineContext;
78-
return CliExit.NotRun(pipelineContext.ParseResult);
79-
}
80-
81-
// @mhutch: I find this more readable than the if conditional version below. There will be at least two more blocks. Look good?
82-
public T? GetValue<T>(CliSymbol symbol)
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)
8383
=> symbol switch
8484
{
8585
{ } when TryGetValue<T>(symbol, out var value)
@@ -89,46 +89,47 @@ protected internal override CliExit Execute(PipelineContext pipelineContext)
8989
// Value was not supplied during parsing, determine default now
9090
{ } when GetDefaultCalculation(symbol) is { } defaultValueCalculation
9191
=> UseValue(symbol, CalculatedDefault<T>(symbol, defaultValueCalculation)),
92-
{ } when TryGetExplicitDefault<T>(symbol, out var explicitValue) => UseValue(symbol, explicitValue),
92+
{ } when TryGetExplicitDefault<T>(symbol, out var explicitValue)
93+
=> UseValue(symbol, explicitValue),
9394
null => throw new ArgumentNullException(nameof(symbol)),
9495
_ => UseValue(symbol, default(T))
9596
};
9697

97-
public T? GetValue2<T>(CliSymbol symbol)
98-
{
99-
if (TryGetValue<T>(symbol, out var value))
100-
{
101-
// It has already been retrieved at least once
102-
return value;
103-
}
104-
if (pipelineContext?.ParseResult?.GetValueResult(symbol) is ValueResult valueResult)
105-
{
106-
// Value was supplied during parsing
107-
return UseValue(symbol, valueResult.GetValue<T>());
108-
}
109-
// Value was not supplied during parsing, determine default now
110-
if (GetDefaultCalculation(symbol) is { } defaultValueCalculation)
111-
{
112-
return UseValue(symbol, CalculatedDefault(symbol, defaultValueCalculation));
113-
}
114-
if (TryGetExplicitDefault<T>(symbol, out var explicitValue))
115-
{
116-
return UseValue(symbol, value);
117-
}
118-
value = default;
119-
SetValue(symbol, value);
120-
return value;
121-
122-
static T? CalculatedDefault(CliSymbol symbol, Func<object?> defaultValueCalculation)
123-
{
124-
var objectValue = defaultValueCalculation();
125-
var value = objectValue is null
126-
? default
127-
: (T)objectValue;
128-
return value;
129-
}
130-
}
131-
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+
//}
132133

133134
private static T? CalculatedDefault<T>(CliSymbol symbol, Func<object?> defaultValueCalculation)
134135
{
@@ -141,9 +142,7 @@ protected internal override CliExit Execute(PipelineContext pipelineContext)
141142

142143
private T? UseValue<T>(CliSymbol symbol, T? value)
143144
{
144-
SetValue(symbol, value);
145+
SetValue<T>(symbol, value);
145146
return value;
146147
}
147-
148-
149148
}

0 commit comments

Comments
 (0)