Skip to content

Commit 35f7ff1

Browse files
KathleenDollardKathleenDollard
committed
Updated Pipeline execution, bug fixes and cleanup
Simple tests now pass.
1 parent ffca21e commit 35f7ff1

File tree

5 files changed

+86
-88
lines changed

5 files changed

+86
-88
lines changed

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

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,31 +30,65 @@ public void Value_is_always_activated()
3030
isActive.Should().BeTrue();
3131
}
3232

33-
[Fact(Skip = "WIP")]
33+
[Fact]
3434
public void ValueSubsystem_returns_values_that_are_entered()
3535
{
3636
var consoleHack = new ConsoleHack().RedirectToBuffer(true);
37-
var pipeline = Pipeline.Create();
3837
CliOption<int> option1 = new CliOption<int>("--intValue");
3938
CliRootCommand rootCommand = [
4039
new CliCommand("x")
4140
{
4241
option1
4342
}];
4443
var configuration = new CliConfiguration(rootCommand);
44+
var pipeline = Pipeline.CreateEmpty();
45+
pipeline.Value = new ValueSubsystem();
4546
const int expected1 = 42;
4647
var input = $"x --intValue {expected1}";
4748

48-
pipeline.Parse(configuration, input);
49+
var parseResult = pipeline.Parse(configuration, input); // assigned for debugging
4950
pipeline.Execute(configuration, input, consoleHack);
5051

5152
pipeline.Value.GetValue<int>(option1).Should().Be(expected1);
5253
}
5354

54-
55-
[Fact(Skip = "WIP")]
55+
[Fact]
5656
public void ValueSubsystem_returns_default_value_when_no_value_is_entered()
5757
{
58+
var consoleHack = new ConsoleHack().RedirectToBuffer(true);
59+
CliOption<int> option1 = new CliOption<int>("--intValue");
60+
CliRootCommand rootCommand = [option1];
61+
var configuration = new CliConfiguration(rootCommand);
62+
var pipeline = Pipeline.CreateEmpty();
63+
pipeline.Value = new ValueSubsystem();
64+
pipeline.Value.DefaultValue.Set(option1, 43);
65+
const int expected1 = 43;
66+
var input = $"";
67+
68+
var parseResult = pipeline.Parse(configuration, input); // assigned for debugging
69+
pipeline.Execute(configuration, input, consoleHack);
70+
71+
pipeline.Value.GetValue<int>(option1).Should().Be(expected1);
72+
}
5873

74+
75+
[Fact]
76+
public void ValueSubsystem_returns_calculated_default_value_when_no_value_is_entered()
77+
{
78+
var consoleHack = new ConsoleHack().RedirectToBuffer(true);
79+
CliOption<int> option1 = new CliOption<int>("--intValue");
80+
CliRootCommand rootCommand = [option1];
81+
var configuration = new CliConfiguration(rootCommand);
82+
var pipeline = Pipeline.CreateEmpty();
83+
pipeline.Value = new ValueSubsystem();
84+
var x = 42;
85+
pipeline.Value.DefaultValueCalculation.Set(option1, () => x + 2);
86+
const int expected1 = 44;
87+
var input = $"";
88+
89+
var parseResult = pipeline.Parse(configuration, input); // assigned for debugging
90+
pipeline.Execute(configuration, input, consoleHack);
91+
92+
pipeline.Value.GetValue<int>(option1).Should().Be(expected1);
5993
}
6094
}

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

Lines changed: 30 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,16 @@
44
using System.CommandLine.Directives;
55
using System.CommandLine.Parsing;
66
using System.CommandLine.Subsystems;
7+
using System.Reflection.PortableExecutable;
78

89
namespace System.CommandLine;
910

1011
public class Pipeline
1112
{
13+
//TODO: When we allow adding subsystems, this code will change
14+
private IEnumerable<CliSubsystem?> Subsystems
15+
=> [Help, Version, Completion, Diagram, Value, ErrorReporting];
16+
1217
public static Pipeline Create(HelpSubsystem? help = null,
1318
VersionSubsystem? version = null,
1419
CompletionSubsystem? completion = null,
@@ -63,61 +68,6 @@ public CliExit Execute(ParseResult parseResult, string rawInput, ConsoleHack? co
6368
return new CliExit(pipelineContext);
6469
}
6570

66-
protected virtual void InitializeHelp(InitializationContext context)
67-
=> Help?.Initialize(context);
68-
69-
protected virtual void InitializeVersion(InitializationContext context)
70-
=> Version?.Initialize(context);
71-
72-
protected virtual void InitializeCompletion(InitializationContext context)
73-
=> Completion?.Initialize(context);
74-
75-
protected virtual void InitializeDiagram(InitializationContext context)
76-
=> Diagram?.Initialize(context);
77-
78-
protected virtual void InitializeErrorReporting(InitializationContext context)
79-
=> ErrorReporting?.Initialize(context);
80-
81-
protected virtual CliExit TearDownHelp(CliExit cliExit)
82-
=> Help is null
83-
? cliExit
84-
: Help.TearDown(cliExit);
85-
86-
protected virtual CliExit? TearDownVersion(CliExit cliExit)
87-
=> Version is null
88-
? cliExit
89-
: Version.TearDown(cliExit);
90-
91-
protected virtual CliExit TearDownCompletion(CliExit cliExit)
92-
=> Completion is null
93-
? cliExit
94-
: Completion.TearDown(cliExit);
95-
96-
protected virtual CliExit TearDownDiagram(CliExit cliExit)
97-
=> Diagram is null
98-
? cliExit
99-
: Diagram.TearDown(cliExit);
100-
101-
protected virtual CliExit TearDownErrorReporting(CliExit cliExit)
102-
=> ErrorReporting is null
103-
? cliExit
104-
: ErrorReporting.TearDown(cliExit);
105-
106-
protected virtual void ExecuteHelp(PipelineContext context)
107-
=> ExecuteIfNeeded(Help, context);
108-
109-
protected virtual void ExecuteVersion(PipelineContext context)
110-
=> ExecuteIfNeeded(Version, context);
111-
112-
protected virtual void ExecuteCompletion(PipelineContext context)
113-
=> ExecuteIfNeeded(Completion, context);
114-
115-
protected virtual void ExecuteDiagram(PipelineContext context)
116-
=> ExecuteIfNeeded(Diagram, context);
117-
118-
protected virtual void ExecuteErrorReporting(PipelineContext context)
119-
=> ExecuteIfNeeded(ErrorReporting, context);
120-
12171
// TODO: Consider whether this should be public. It would simplify testing, but would it do anything else
12272
// TODO: Confirm that it is OK for ConsoleHack to be unavailable in Initialize
12373
/// <summary>
@@ -131,11 +81,13 @@ protected virtual void ExecuteErrorReporting(PipelineContext context)
13181
/// </remarks>
13282
protected virtual void InitializeSubsystems(InitializationContext context)
13383
{
134-
InitializeHelp(context);
135-
InitializeVersion(context);
136-
InitializeCompletion(context);
137-
InitializeDiagram(context);
138-
InitializeErrorReporting(context);
84+
foreach (var subsystem in Subsystems)
85+
{
86+
if ( subsystem is not null)
87+
{
88+
subsystem.Initialize(context);
89+
}
90+
}
13991
}
14092

14193
// TODO: Consider whether this should be public
@@ -144,26 +96,31 @@ protected virtual void InitializeSubsystems(InitializationContext context)
14496
/// Perform any cleanup operations
14597
/// </summary>
14698
/// <param name="pipelineContext">The context of the current execution</param>
147-
/// <remarks>
148-
/// Note to inheritors: The ordering of tear down should normally be in the reverse order than initializing
149-
/// </remarks>
15099
protected virtual CliExit TearDownSubsystems(CliExit cliExit)
151100
{
152-
TearDownErrorReporting(cliExit);
153-
TearDownDiagram(cliExit);
154-
TearDownCompletion(cliExit);
155-
TearDownVersion(cliExit);
156-
TearDownHelp(cliExit);
101+
// TODO: Work on this design as the last cliExit wins and they may not all be well behaved
102+
var subsystems = Subsystems.Reverse();
103+
foreach (var subsystem in subsystems)
104+
{
105+
if (subsystem is not null)
106+
{
107+
cliExit = subsystem.TearDown(cliExit);
108+
}
109+
}
157110
return cliExit;
158111
}
159112

160113
protected virtual void ExecuteSubsystems(PipelineContext pipelineContext)
161114
{
162-
ExecuteHelp(pipelineContext);
163-
ExecuteVersion(pipelineContext);
164-
ExecuteCompletion(pipelineContext);
165-
ExecuteDiagram(pipelineContext);
166-
ExecuteErrorReporting(pipelineContext);
115+
// TODO: Consider redesign where pipelineContext is not modifiable.
116+
//
117+
foreach (var subsystem in Subsystems)
118+
{
119+
if (subsystem is not null)
120+
{
121+
pipelineContext = subsystem.ExecuteIfNeeded(pipelineContext);
122+
}
123+
}
167124
}
168125

169126
protected static void ExecuteIfNeeded(CliSubsystem? subsystem, PipelineContext pipelineContext)

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,8 @@ protected internal void SetAnnotation<TValue>(CliSymbol symbol, AnnotationId<TVa
7676
/// </summary>
7777
/// <param name="pipelineContext">The context contains data like the ParseResult, and allows setting of values like whether execution was handled and the CLI should terminate </param>
7878
/// <returns>A CliExit object with information such as whether the CLI should terminate</returns>
79-
protected internal virtual CliExit Execute(PipelineContext pipelineContext) => CliExit.NotRun(pipelineContext.ParseResult);
79+
protected internal virtual CliExit Execute(PipelineContext pipelineContext)
80+
=> CliExit.NotRun(pipelineContext.ParseResult);
8081

8182
internal PipelineContext ExecuteIfNeeded(PipelineContext pipelineContext)
8283
=> ExecuteIfNeeded(pipelineContext.ParseResult, pipelineContext);
@@ -114,6 +115,7 @@ internal PipelineContext ExecuteIfNeeded(ParseResult? parseResult, PipelineConte
114115
/// <param name="configuration">The CLI configuration, which contains the RootCommand for customization</param>
115116
/// <returns>True if parsing should continue</returns> // there might be a better design that supports a message
116117
// TODO: Because of this and similar usage, consider combining CLI declaration and config. ArgParse calls this the parser, which I like
118+
// TODO: Why does Intitialize return a configuration?
117119
protected internal virtual CliConfiguration Initialize(InitializationContext context)
118120
=> context.Configuration;
119121

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

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,9 @@ public class ValueSubsystem : CliSubsystem
1111
{
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.
14-
private PipelineContext? pipelineContext = null;
14+
//private PipelineContext? pipelineContext = null;
1515
private Dictionary<CliSymbol, object?> cachedValues = new();
16+
private ParseResult? parseResult = null;
1617

1718
public ValueSubsystem(IAnnotationProvider? annotationProvider = null)
1819
: base(ValueAnnotations.Prefix, SubsystemKind.Version, annotationProvider)
@@ -26,7 +27,7 @@ public ValueSubsystem(IAnnotationProvider? annotationProvider = null)
2627
// : "";
2728
private bool TryGetDefaultValue<T>(CliSymbol symbol, out T? defaultValue)
2829
{
29-
if (TryGetAnnotation(symbol, ValueAnnotations.Value, out var objectValue))
30+
if (TryGetAnnotation(symbol, ValueAnnotations.DefaultValue, out var objectValue))
3031
{
3132
defaultValue = (T)objectValue;
3233
return true;
@@ -47,15 +48,17 @@ public AnnotationAccessor<Func<object?>?> DefaultValueCalculation
4748
=> new(this, ValueAnnotations.DefaultValueCalculation);
4849

4950
protected internal override bool GetIsActivated(ParseResult? parseResult)
50-
=> true;
51+
{
52+
this.parseResult = parseResult;
53+
return true;
54+
}
5155

5256
protected internal override CliExit Execute(PipelineContext pipelineContext)
5357
{
54-
this.pipelineContext = pipelineContext;
55-
return CliExit.NotRun(pipelineContext.ParseResult);
58+
parseResult ??= pipelineContext.ParseResult;
59+
return base.Execute(pipelineContext);
5660
}
5761

58-
5962
// TODO: Do it! Consider using a simple dictionary instead of the annotation (@mhutch) because with is not useful here
6063
private void SetValue<T>(CliSymbol symbol, object? value)
6164
=> cachedValues.Add(symbol, value);
@@ -82,9 +85,9 @@ private bool TryGetValue<T>(CliSymbol symbol, out T? value)
8285
{
8386
not null when TryGetValue<T>(symbol, out var value)
8487
=> value, // It has already been retrieved at least once
85-
CliArgument argument when pipelineContext?.ParseResult?.GetValueResult(argument) is ValueResult valueResult // GetValue would always return a value
88+
CliArgument argument when parseResult?.GetValueResult(argument) is { } valueResult // GetValue not used because it would always return a value
8689
=> 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
90+
CliOption option when parseResult?.GetValueResult(option) is {} valueResult // GetValue not used because it would always return a value
8891
=> UseValue(symbol, valueResult.GetValue<T>()), // Value was supplied during parsing
8992
// Value was not supplied during parsing, determine default now
9093
not null when DefaultValueCalculation.TryGet(symbol, out var defaultValueCalculation)

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@ internal ValueResult(
2626
internal object? Value { get; }
2727

2828
public T? GetValue<T>()
29-
=> (T?)Value;
29+
=> Value is null
30+
? default
31+
: (T?)Value;
3032

3133
// This needs to be a collection because collection types have multiple tokens and they will not be simple offsets when response files are used
3234
// TODO: Consider more efficient ways to do this in the case where there is a single location

0 commit comments

Comments
 (0)