Skip to content

Commit dabfb02

Browse files
KathleenDollardKathleenDollard
committed
Response to PR Review meeting
1 parent b878eba commit dabfb02

File tree

8 files changed

+134
-148
lines changed

8 files changed

+134
-148
lines changed

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,13 @@
33

44
using System.CommandLine.Subsystems;
55
using System.CommandLine.Subsystems.Annotations;
6-
using System.Reflection.Metadata.Ecma335;
76

87
namespace System.CommandLine;
98

109
public class CompletionSubsystem : CliSubsystem
1110
{
1211
public CompletionSubsystem(IAnnotationProvider? annotationProvider = null)
13-
: base(CompletionAnnotations.Prefix, SubsystemKind.Completion, annotationProvider)
12+
: base(CompletionAnnotations.Prefix, SubsystemKind.Completion, annotationProvider)
1413
{ }
1514

1615
// TODO: Figure out trigger for completions

‎src/System.CommandLine.Subsystems/Directives/DiagramSubsystem.cs

Lines changed: 85 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -70,113 +70,113 @@ private static void Diagram(
7070
*/
7171
// TODO: Directives
7272
/*
73-
switch (symbolResult)
74-
{
75-
case DirectiveResult { Directive: not DiagramDirective }:
76-
break;
77-
*/
78-
79-
// TODO: This logic is deeply tied to internal types/properties. These aren't things we probably want to expose like SymbolNode. See #2349 for alternatives
80-
/*
81-
case ArgumentResult argumentResult:
73+
switch (symbolResult)
8274
{
83-
var includeArgumentName =
84-
argumentResult.Argument.FirstParent!.Symbol is CliCommand { HasArguments: true, Arguments.Count: > 1 };
85-
86-
if (includeArgumentName)
87-
{
88-
builder.Append("[ ");
89-
builder.Append(argumentResult.Argument.Name);
90-
builder.Append(' ');
91-
}
75+
case DirectiveResult { Directive: not DiagramDirective }:
76+
break;
77+
*/
9278

93-
if (argumentResult.Argument.Arity.MaximumNumberOfValues > 0)
94-
{
95-
ArgumentConversionResult conversionResult = argumentResult.GetArgumentConversionResult();
96-
switch (conversionResult.Result)
79+
// TODO: This logic is deeply tied to internal types/properties. These aren't things we probably want to expose like SymbolNode. See #2349 for alternatives
80+
/*
81+
case ArgumentResult argumentResult:
9782
{
98-
case ArgumentConversionResultType.NoArgument:
99-
break;
100-
case ArgumentConversionResultType.Successful:
101-
switch (conversionResult.Value)
83+
var includeArgumentName =
84+
argumentResult.Argument.FirstParent!.Symbol is CliCommand { HasArguments: true, Arguments.Count: > 1 };
85+
86+
if (includeArgumentName)
87+
{
88+
builder.Append("[ ");
89+
builder.Append(argumentResult.Argument.Name);
90+
builder.Append(' ');
91+
}
92+
93+
if (argumentResult.Argument.Arity.MaximumNumberOfValues > 0)
94+
{
95+
ArgumentConversionResult conversionResult = argumentResult.GetArgumentConversionResult();
96+
switch (conversionResult.Result)
10297
{
103-
case string s:
104-
builder.Append($"<{s}>");
98+
case ArgumentConversionResultType.NoArgument:
10599
break;
100+
case ArgumentConversionResultType.Successful:
101+
switch (conversionResult.Value)
102+
{
103+
case string s:
104+
builder.Append($"<{s}>");
105+
break;
106+
107+
case IEnumerable items:
108+
builder.Append('<');
109+
builder.Append(
110+
string.Join("> <",
111+
items.Cast<object>().ToArray()));
112+
builder.Append('>');
113+
break;
114+
115+
default:
116+
builder.Append('<');
117+
builder.Append(conversionResult.Value);
118+
builder.Append('>');
119+
break;
120+
}
106121
107-
case IEnumerable items:
108-
builder.Append('<');
109-
builder.Append(
110-
string.Join("> <",
111-
items.Cast<object>().ToArray()));
112-
builder.Append('>');
113122
break;
114123
115-
default:
124+
default: // failures
116125
builder.Append('<');
117-
builder.Append(conversionResult.Value);
126+
builder.Append(string.Join("> <", symbolResult.Tokens.Select(t => t.Value)));
118127
builder.Append('>');
128+
119129
break;
120130
}
131+
}
121132
122-
break;
123-
124-
default: // failures
125-
builder.Append('<');
126-
builder.Append(string.Join("> <", symbolResult.Tokens.Select(t => t.Value)));
127-
builder.Append('>');
133+
if (includeArgumentName)
134+
{
135+
builder.Append(" ]");
136+
}
128137
129-
break;
138+
break;
130139
}
131-
}
132-
133-
if (includeArgumentName)
134-
{
135-
builder.Append(" ]");
136-
}
137-
138-
break;
139-
}
140140
141-
default:
142-
{
143-
OptionResult? optionResult = symbolResult as OptionResult;
144-
145-
if (optionResult is { Implicit: true })
146-
{
147-
builder.Append('*');
148-
}
141+
default:
142+
{
143+
OptionResult? optionResult = symbolResult as OptionResult;
144+
145+
if (optionResult is { Implicit: true })
146+
{
147+
builder.Append('*');
148+
}
149+
150+
builder.Append("[ ");
151+
152+
if (optionResult is not null)
153+
{
154+
builder.Append(optionResult.IdentifierToken?.Value ?? optionResult.Option.Name);
155+
}
156+
else
157+
{
158+
builder.Append(((CommandResult)symbolResult).IdentifierToken.Value);
159+
}
160+
161+
foreach (SymbolResult child in symbolResult.SymbolResultTree.GetChildren(symbolResult))
162+
{
163+
if (child is ArgumentResult arg &&
164+
(arg.Argument.ValueType == typeof(bool) ||
165+
arg.Argument.Arity.MaximumNumberOfValues == 0))
166+
{
167+
continue;
168+
}
149169
150-
builder.Append("[ ");
170+
builder.Append(' ');
151171
152-
if (optionResult is not null)
153-
{
154-
builder.Append(optionResult.IdentifierToken?.Value ?? optionResult.Option.Name);
155-
}
156-
else
157-
{
158-
builder.Append(((CommandResult)symbolResult).IdentifierToken.Value);
159-
}
172+
Diagram(builder, child, parseResult);
173+
}
160174
161-
foreach (SymbolResult child in symbolResult.SymbolResultTree.GetChildren(symbolResult))
162-
{
163-
if (child is ArgumentResult arg &&
164-
(arg.Argument.ValueType == typeof(bool) ||
165-
arg.Argument.Arity.MaximumNumberOfValues == 0))
166-
{
167-
continue;
175+
builder.Append(" ]");
176+
break;
168177
}
169-
170-
builder.Append(' ');
171-
172-
Diagram(builder, child, parseResult);
173178
}
174-
175-
builder.Append(" ]");
176-
break;
177179
}
178180
}
179-
}
180-
}
181181
*/
182182
}

‎src/System.CommandLine.Subsystems/Directives/DirectiveSubsystem.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ public abstract class DirectiveSubsystem : CliSubsystem
1313
public string Id { get; }
1414
public Location? Location { get; private set; }
1515

16-
public DirectiveSubsystem(string name, SubsystemKind kind, IAnnotationProvider? annotationProvider = null, string? id = null)
16+
public DirectiveSubsystem(string name, SubsystemKind kind, IAnnotationProvider? annotationProvider = null, string? id = null)
1717
: base(name, kind, annotationProvider: annotationProvider)
1818
{
1919
Id = id ?? name;

‎src/System.CommandLine.Subsystems/Directives/ResponseSubsystem.cs

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -16,26 +16,25 @@ protected internal override void Initialize(InitializationContext context)
1616

1717
public (List<string>? tokens, List<string>? errors) Replacer(string responseSourceName)
1818
{
19-
if (Enabled)
19+
if (!Enabled)
2020
{
21-
try
22-
{
23-
// TODO: Include checks from previous system.
24-
var contents = File.ReadAllText(responseSourceName);
25-
return (CliParser.SplitCommandLine(contents).ToList(), null);
26-
}
27-
catch
28-
{
29-
// TODO: Switch to proper errors
30-
return (null,
31-
errors:
32-
[
33-
$"Failed to open response file {responseSourceName}"
34-
]);
35-
}
21+
return ([responseSourceName], null);
22+
}
23+
try
24+
{
25+
// TODO: Include checks from previous system.
26+
var contents = File.ReadAllText(responseSourceName);
27+
return (CliParser.SplitCommandLine(contents).ToList(), null);
28+
}
29+
catch
30+
{
31+
// TODO: Switch to proper errors
32+
return (null,
33+
errors:
34+
[
35+
$"Failed to open response file {responseSourceName}"
36+
]);
3637
}
37-
// TODO: Confirm this is not an error state
38-
return ([responseSourceName], null);
3938
}
4039

4140
// TODO: File handling from previous system - ensure these checks are done (note: no tests caught these oversights

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

Lines changed: 14 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,11 @@ public partial class Pipeline
2121
/// <summary>
2222
/// Creates an instance of the pipeline using standard features.
2323
/// </summary>
24-
/// <param name="help">A help subsystem to replace the standard one. To add a subsystem, use <cref>AddSubsystem.</cref></param>
25-
/// <param name="version">A help subsystem to replace the standard one. To add a subsystem, use <cref>AddSubsystem.</param>
26-
/// <param name="completion">A help subsystem to replace the standard one. To add a subsystem, use <cref>AddSubsystem.</param>
27-
/// <param name="diagram">A help subsystem to replace the standard one. To add a subsystem, use <cref>AddSubsystem.</param>
28-
/// <param name="errorReporting">A help subsystem to replace the standard one. To add a subsystem, use <cref>AddSubsystem.</param>
24+
/// <param name="help">A help subsystem to replace the standard one. To add a subsystem, use <see cref="AddSubsystem"></param>
25+
/// <param name="version">A help subsystem to replace the standard one. To add a subsystem, use <see cref="AddSubsystem"></param>
26+
/// <param name="completion">A help subsystem to replace the standard one. To add a subsystem, use <see cref="AddSubsystem"></param>
27+
/// <param name="diagram">A help subsystem to replace the standard one. To add a subsystem, use <see cref="AddSubsystem"></param>
28+
/// <param name="errorReporting">A help subsystem to replace the standard one. To add a subsystem, use <see cref="AddSubsystem"></param>
2929
/// <returns>A new pipeline.</returns>
3030
/// <remarks>
3131
/// Currently, the standard <see cref="ValueSubsystem"/>, <see cref="ValidationSubsystem"/> , and <see cref="ResponseSubsystem"/> cannot be replaced. <see cref="ResponseSubsystem"/> is disabled by default.
@@ -89,9 +89,9 @@ public bool ResponseFilesEnabled
8989
/// <remarks>
9090
/// The phase in which the subsystem runs is determined by the subsystem's 'Kind' property.
9191
/// <br/>
92-
/// To replace one of hte standard subsystems, use the `Pipeline.(subsystem)` property, such as `myPipeline.Help = new OtherHelp();`
92+
/// To replace one of the standard subsystems, use the `Pipeline.(subsystem)` property, such as `myPipeline.Help = new OtherHelp();`
9393
/// </remarks>
94-
public void AddSubsystem(CliSubsystem subsystem, PhaseTiming timing = PhaseTiming.Before)
94+
public void AddSubsystem(CliSubsystem subsystem, AddToPhaseBehavior timing = AddToPhaseBehavior.SubsystemRecommendation)
9595
{
9696
switch (subsystem.Kind)
9797
{
@@ -111,6 +111,8 @@ public void AddSubsystem(CliSubsystem subsystem, PhaseTiming timing = PhaseTimin
111111
case SubsystemKind.Version:
112112
versionPhase.AddSubsystem(subsystem, timing);
113113
break;
114+
// You can add Validation and Invocation subsystems, but you can't remove the core.
115+
// Other things may need to be run in the phase.
114116
case SubsystemKind.Validation:
115117
validationPhase.AddSubsystem(subsystem, timing);
116118
break;
@@ -172,21 +174,13 @@ public ErrorReportingSubsystem? ErrorReporting
172174
/// <summary>
173175
/// Sets or gets the validation subsystem
174176
/// </summary>
175-
public ValidationSubsystem? Validation
176-
{
177-
get => validationPhase.Subsystem;
178-
set => validationPhase.Subsystem = value;
179-
}
177+
public ValidationSubsystem? Validation { get; }
180178

181179
// TODO: Consider whether replacing the invocation subsystem is valuable
182180
/// <summary>
183181
/// Sets or gets the invocation subsystem
184182
/// </summary>
185-
public InvocationSubsystem? Invocation
186-
{
187-
get => invocationPhase.Subsystem;
188-
set => invocationPhase.Subsystem = value;
189-
}
183+
public InvocationSubsystem? Invocation { get; }
190184

191185
/// <summary>
192186
/// Gets the value subsystem which manages entered and default values.
@@ -222,6 +216,7 @@ public PipelineResult Execute(ParseResult parseResult, string rawInput, ConsoleH
222216
// TODO: Allow subsystems to control short-circuiting
223217
foreach (var subsystem in phase.GetSubsystems())
224218
{
219+
// TODO: RunEvenIfAlreadyHandled needs more thought and laying out the scenarios
225220
if (subsystem is not null && (!pipelineResult.AlreadyHandled || subsystem.RunsEvenIfAlreadyHandled))
226221
{
227222
subsystem.ExecuteIfNeeded(pipelineResult);
@@ -250,7 +245,7 @@ protected virtual void InitializeSubsystems(InitializationContext context)
250245
// TODO: Allow subsystems to control short-circuiting? Not sure we need that for initialization
251246
foreach (var subsystem in phase.GetSubsystems())
252247
{
253-
subsystem?.Initialize(context);
248+
subsystem.Initialize(context);
254249
}
255250
}
256251
}
@@ -269,7 +264,7 @@ protected virtual void TearDownSubsystems(PipelineResult pipelineResult)
269264
// TODO: Allow subsystems to control short-circuiting? Not sure we need that for teardown
270265
foreach (var subsystem in phase.GetSubsystems())
271266
{
272-
subsystem?.TearDown(pipelineResult);
267+
subsystem.TearDown(pipelineResult);
273268
}
274269
}
275270
}

‎src/System.CommandLine.Subsystems/Subsystems/PhaseTiming.cs renamed to ‎src/System.CommandLine.Subsystems/Subsystems/AddToPhaseBehavior.cs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,9 @@
33

44
namespace System.CommandLine.Subsystems;
55

6-
public enum PhaseTiming
6+
public enum AddToPhaseBehavior
77
{
8-
Before = 0,
9-
After
8+
SubsystemRecommendation = 0,
9+
Prepend,
10+
Append
1011
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ namespace System.CommandLine.Subsystems;
1212
/// <param name="annotationProvider"></param>
1313
public abstract class CliSubsystem
1414
{
15-
protected CliSubsystem(string name, SubsystemKind subsystemKind, IAnnotationProvider? annotationProvider)
15+
protected CliSubsystem(string name, SubsystemKind subsystemKind, IAnnotationProvider? annotationProvider)
1616
{
1717
Name = name;
1818
_annotationProvider = annotationProvider;
@@ -28,6 +28,7 @@ protected CliSubsystem(string name, SubsystemKind subsystemKind, IAnnotationPro
2828
/// Defines the kind of subsystem, such as help or version
2929
/// </summary>
3030
public SubsystemKind Kind { get; }
31+
public AddToPhaseBehavior RecommendedAddToPhaseBehavior { get; }
3132

3233
private readonly IAnnotationProvider? _annotationProvider;
3334

0 commit comments

Comments
 (0)