Merge pull request #2444 from KathleenDollard/ph-kad-rearrange-commits

Reworked pipeline to introduce phases for ordering

Author: KathleenDollard

src/System.CommandLine.Subsystems.Tests/ResponseSubsystemTests.cs

@@ -19,6 +19,7 @@ public void Simple_response_file_contributes_to_parsing()
         var rootCommand = new CliRootCommand { option };
         var configuration = new CliConfiguration(rootCommand);
         var subsystem = new ResponseSubsystem();
+        subsystem.Enabled = true;
         string[] args = ["@Response_1.rsp"];
 
         Subsystem.Initialize(subsystem, configuration, args);

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

@@ -29,7 +29,7 @@ public void ValueSubsystem_is_activated_by_default()
 
         isActive.Should().BeTrue();
     }
-
+    /* Hold these tests until we determine if ValueSubsystem is replaceable
     [Fact]
     public void ValueSubsystem_returns_values_that_are_entered()
     {
@@ -90,4 +90,5 @@ public void ValueSubsystem_returns_calculated_default_value_when_no_value_is_ent
 
         pipeline.Value.GetValue<int>(option).Should().Be(expected);
     }
+    */
 }

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

@@ -7,7 +7,7 @@
 
 namespace System.CommandLine.Directives;
 
-public class DiagramSubsystem( IAnnotationProvider? annotationProvider = null)
+public class DiagramSubsystem(IAnnotationProvider? annotationProvider = null)
     : DirectiveSubsystem("diagram", SubsystemKind.Diagram, annotationProvider)
 {
     //protected internal override bool GetIsActivated(ParseResult? parseResult)
@@ -68,8 +68,8 @@ private static void Diagram(
             builder.Append('!');
         }
     */
-// TODO: Directives
-/*
+    // TODO: Directives
+    /*
         switch (symbolResult)
         {
             case DirectiveResult { Directive: not DiagramDirective }:

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

@@ -9,11 +9,17 @@ namespace System.CommandLine.Directives;
 public class ResponseSubsystem()
     : CliSubsystem("Response", SubsystemKind.Response, null)
 {
-    protected internal override void Initialize(InitializationContext context) 
+    public bool Enabled { get; set; }
+
+    protected internal override void Initialize(InitializationContext context)
         => context.Configuration.ResponseFileTokenReplacer = Replacer;
 
-    public static (List<string>? tokens, List<string>? errors) Replacer(string responseSourceName)
+    public (List<string>? tokens, List<string>? errors) Replacer(string responseSourceName)
     {
+        if (!Enabled)
+        {
+            return ([responseSourceName], null);
+        }
         try
         {
             // TODO: Include checks from previous system.

src/System.CommandLine.Subsystems/HelpSubsystem.cs

@@ -24,7 +24,7 @@ public class HelpSubsystem(IAnnotationProvider? annotationProvider = null)
         Arity = ArgumentArity.Zero
     };
 
-    protected internal override void Initialize(InitializationContext context) 
+    protected internal override void Initialize(InitializationContext context)
         => context.Configuration.RootCommand.Add(HelpOption);
 
     protected internal override bool GetIsActivated(ParseResult? parseResult)
@@ -37,6 +37,6 @@ protected internal override void Execute(PipelineResult pipelineResult)
         pipelineResult.SetSuccess();
     }
 
-    public bool TryGetDescription (CliSymbol symbol, out string? description)
-        => TryGetAnnotation (symbol, HelpAnnotations.Description, out description);
+    public bool TryGetDescription(CliSymbol symbol, out string? description)
+        => TryGetAnnotation(symbol, HelpAnnotations.Description, out description);
 }

src/System.CommandLine.Subsystems/InvocationSubsystem.cs

@@ -0,0 +1,11 @@
+// Copyright (c) .NET Foundation and contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System.CommandLine.Subsystems;
+using System.CommandLine.Subsystems.Annotations;
+
+namespace System.CommandLine;
+
+public class InvocationSubsystem(IAnnotationProvider? annotationProvider = null)
+    : CliSubsystem(InvocationAnnotations.Prefix, SubsystemKind.Invocation, annotationProvider)
+{}

src/System.CommandLine.Subsystems/Pipeline.cs

@@ -7,39 +7,190 @@
 
 namespace System.CommandLine;
 
-public class Pipeline
+public partial class Pipeline
 {
-    //TODO:  When we allow adding subsystems, this code will change
-    private IEnumerable<CliSubsystem?> Subsystems
-        => [Help, Version, Completion, Diagram, Value, ErrorReporting];
+    private readonly PipelinePhase<DiagramSubsystem> diagramPhase = new(SubsystemKind.Diagram);
+    private readonly PipelinePhase<CompletionSubsystem> completionPhase = new(SubsystemKind.Completion);
+    private readonly PipelinePhase<HelpSubsystem> helpPhase = new(SubsystemKind.Help);
+    private readonly PipelinePhase<VersionSubsystem> versionPhase = new(SubsystemKind.Version);
+    private readonly PipelinePhase<ValidationSubsystem> validationPhase = new(SubsystemKind.Validation);
+    private readonly PipelinePhase<InvocationSubsystem> invocationPhase = new(SubsystemKind.Invocation);
+    private readonly PipelinePhase<ErrorReportingSubsystem> errorReportingPhase = new(SubsystemKind.ErrorReporting);
+    private readonly IEnumerable<PipelinePhase> phases;
 
+    /// <summary>
+    /// Creates an instance of the pipeline using standard features.
+    /// </summary>
+    /// <param name="help">A help subsystem to replace the standard one. To add a subsystem, use <see cref="AddSubsystem"></param>
+    /// <param name="version">A help subsystem to replace the standard one. To add a subsystem, use <see cref="AddSubsystem"></param>
+    /// <param name="completion">A help subsystem to replace the standard one. To add a subsystem, use <see cref="AddSubsystem"></param>
+    /// <param name="diagram">A help subsystem to replace the standard one. To add a subsystem, use <see cref="AddSubsystem"></param>
+    /// <param name="errorReporting">A help subsystem to replace the standard one. To add a subsystem, use <see cref="AddSubsystem"></param>
+    /// <returns>A new pipeline.</returns>
+    /// <remarks>
+    /// Currently, the standard <see cref="ValueSubsystem"/>, <see cref="ValidationSubsystem"/> , and <see cref="ResponseSubsystem"/> cannot be replaced. <see cref="ResponseSubsystem"/> is disabled by default.
+    /// </remarks>
     public static Pipeline Create(HelpSubsystem? help = null,
                                   VersionSubsystem? version = null,
                                   CompletionSubsystem? completion = null,
                                   DiagramSubsystem? diagram = null,
-                                  ErrorReportingSubsystem? errorReporting = null,
-                                  ValueSubsystem? value = null)
+                                  ErrorReportingSubsystem? errorReporting = null)
         => new()
         {
             Help = help ?? new HelpSubsystem(),
             Version = version ?? new VersionSubsystem(),
             Completion = completion ?? new CompletionSubsystem(),
             Diagram = diagram ?? new DiagramSubsystem(),
             ErrorReporting = errorReporting ?? new ErrorReportingSubsystem(),
-            Value = value ?? new ValueSubsystem()
         };
 
+    /// <summary>
+    /// Creates an instance of the pipeline with no features. Use this if you want to explicitly add features.
+    /// </summary>
+    /// <returns>A new pipeline.</returns>
+    /// <remarks>
+    /// The <cref>ValueSubsystem</cref> and <see cref="ResponseSubsystem"/> is always added and cannot be changed.
+    /// </remarks>
     public static Pipeline CreateEmpty()
         => new();
 
-    private Pipeline() { }
+    private Pipeline()
+    {
+        Value = new ValueSubsystem();
+        Response = new ResponseSubsystem();
+        Invocation = new InvocationSubsystem();
+        Validation = new ValidationSubsystem();
+
+        // This order is based on: if the user entered both, which should they get?
+        // * It is reasonable to diagram help and completion. More reasonable than getting help on Diagram or Completion
+        // * A future version of Help and Version may take arguments/options. In that case, help on version is reasonable.
+        phases =
+        [
+            diagramPhase, completionPhase, helpPhase, versionPhase,
+            validationPhase, invocationPhase, errorReportingPhase
+        ];
+    }
+
+    /// <summary>
+    /// Enables response files. They are disabled by default.
+    /// </summary>
+    public bool ResponseFilesEnabled
+    {
+        get => Response.Enabled;
+        set => Response.Enabled = value;
+    }
+
+    /// <summary>
+    /// Adds a subsystem.
+    /// </summary>
+    /// <param name="subsystem">The subsystem to add.</param>
+    /// <param name="timing"><see cref="PhaseTiming.Before"/> indicates that the subsystem should run before all other subsystems in the phase, and <see cref="PhaseTiming.After"/> indicates it should run after other subsystems. The default is <see cref="PhaseTiming.Before"/>.</param>
+    /// <exception cref="InvalidOperationException"></exception>
+    /// <remarks>
+    /// The phase in which the subsystem runs is determined by the subsystem's 'Kind' property.
+    /// <br/>
+    /// To replace one of the standard subsystems, use the `Pipeline.(subsystem)` property, such as `myPipeline.Help = new OtherHelp();`
+    /// </remarks>
+    public void AddSubsystem(CliSubsystem subsystem, AddToPhaseBehavior timing = AddToPhaseBehavior.SubsystemRecommendation)
+    {
+        switch (subsystem.Kind)
+        {
+            case SubsystemKind.Other:
+            case SubsystemKind.Response:
+            case SubsystemKind.Value:
+                throw new InvalidOperationException($"You cannot add subsystems to {subsystem.Kind}");
+            case SubsystemKind.Diagram:
+                diagramPhase.AddSubsystem(subsystem, timing);
+                break;
+            case SubsystemKind.Completion:
+                completionPhase.AddSubsystem(subsystem, timing);
+                break;
+            case SubsystemKind.Help:
+                helpPhase.AddSubsystem(subsystem, timing);
+                break;
+            case SubsystemKind.Version:
+                versionPhase.AddSubsystem(subsystem, timing);
+                break;
+            // You can add Validation and Invocation subsystems, but you can't remove the core.
+            // Other things may need to be run in the phase.
+            case SubsystemKind.Validation:
+                validationPhase.AddSubsystem(subsystem, timing);
+                break;
+            case SubsystemKind.Invocation:
+                invocationPhase.AddSubsystem(subsystem, timing);
+                break;
+            case SubsystemKind.ErrorReporting:
+                errorReportingPhase.AddSubsystem(subsystem, timing);
+                break;
+        }
+    }
+
+    /// <summary>
+    /// Sets or gets the diagramming subsystem.
+    /// </summary>
+    public DiagramSubsystem? Diagram
+    {
+        get => diagramPhase.Subsystem;
+        set => diagramPhase.Subsystem = value;
+    }
 
-    public HelpSubsystem? Help { get; set; }
-    public VersionSubsystem? Version { get; set; }
-    public CompletionSubsystem? Completion { get; set; }
-    public DiagramSubsystem? Diagram { get; set; }
-    public ErrorReportingSubsystem? ErrorReporting { get; set; }
-    public ValueSubsystem? Value { get; set; }
+    /// <summary>
+    /// Sets or gets the completion subsystem.
+    /// </summary>
+    public CompletionSubsystem? Completion
+    {
+        get => completionPhase.Subsystem;
+        set => completionPhase.Subsystem = value;
+    }
+
+    /// <summary>
+    /// Sets or gets the help subsystem.
+    /// </summary>
+    public HelpSubsystem? Help
+    {
+        get => helpPhase.Subsystem;
+        set => helpPhase.Subsystem = value;
+    }
+
+    /// <summary>
+    /// Sets or gets the version subsystem.
+    /// </summary>
+    public VersionSubsystem? Version
+    {
+        get => versionPhase.Subsystem;
+        set => versionPhase.Subsystem = value;
+    }
+
+    /// <summary>
+    /// Sets or gets the error reporting subsystem.
+    /// </summary>
+    public ErrorReportingSubsystem? ErrorReporting
+    {
+        get => errorReportingPhase.Subsystem;
+        set => errorReportingPhase.Subsystem = value;
+    }
+
+    // TODO: Consider whether replacing the validation subsystem is valuable
+    /// <summary>
+    /// Sets or gets the validation subsystem
+    /// </summary>
+    public ValidationSubsystem? Validation { get; }
+
+    // TODO: Consider whether replacing the invocation subsystem is valuable
+    /// <summary>
+    /// Sets or gets the invocation subsystem
+    /// </summary>
+    public InvocationSubsystem? Invocation { get; }
+
+    /// <summary>
+    /// Gets the value subsystem which manages entered and default values.
+    /// </summary>
+    public ValueSubsystem Value { get; }
+
+    /// <summary>
+    /// Gets the response file subsystem
+    /// </summary>
+    public ResponseSubsystem Response { get; }
 
     public ParseResult Parse(CliConfiguration configuration, string rawInput)
         => Parse(configuration, CliParser.SplitCommandLine(rawInput).ToArray());
@@ -55,16 +206,24 @@ public PipelineResult Execute(CliConfiguration configuration, string rawInput, C
         => Execute(configuration, CliParser.SplitCommandLine(rawInput).ToArray(), rawInput, consoleHack);
 
     public PipelineResult Execute(CliConfiguration configuration, string[] args, string rawInput, ConsoleHack? consoleHack = null)
-    {
-        var pipelineResult = Execute(Parse(configuration, args), rawInput, consoleHack);
-        TearDownSubsystems(pipelineResult);
-        return pipelineResult;
-    }
+        => Execute(Parse(configuration, args), rawInput, consoleHack);
 
     public PipelineResult Execute(ParseResult parseResult, string rawInput, ConsoleHack? consoleHack = null)
     {
         var pipelineResult = new PipelineResult(parseResult, rawInput, this, consoleHack ?? new ConsoleHack());
-        ExecuteSubsystems(pipelineResult);
+        foreach (var phase in phases)
+        {
+            // TODO: Allow subsystems to control short-circuiting
+            foreach (var subsystem in phase.GetSubsystems())
+            {
+                // TODO: RunEvenIfAlreadyHandled needs more thought and laying out the scenarios
+                if (subsystem is not null && (!pipelineResult.AlreadyHandled || subsystem.RunsEvenIfAlreadyHandled))
+                {
+                    subsystem.ExecuteIfNeeded(pipelineResult);
+                }
+            }
+        }
+        TearDownSubsystems(pipelineResult);
         return pipelineResult;
     }
 
@@ -81,9 +240,10 @@ public PipelineResult Execute(ParseResult parseResult, string rawInput, ConsoleH
     /// </remarks>
     protected virtual void InitializeSubsystems(InitializationContext context)
     {
-        foreach (var subsystem in Subsystems)
+        foreach (var phase in phases)
         {
-            if (subsystem is not null)
+            // TODO: Allow subsystems to control short-circuiting? Not sure we need that for initialization
+            foreach (var subsystem in phase.GetSubsystems())
             {
                 subsystem.Initialize(context);
             }
@@ -99,35 +259,13 @@ protected virtual void InitializeSubsystems(InitializationContext context)
     protected virtual void TearDownSubsystems(PipelineResult pipelineResult)
     {
         // TODO: Work on this design as the last pipelineResult wins and they may not all be well behaved
-        var subsystems = Subsystems.Reverse();
-        foreach (var subsystem in subsystems)
+        foreach (var phase in phases)
         {
-            if (subsystem is not null)
+            // TODO: Allow subsystems to control short-circuiting? Not sure we need that for teardown
+            foreach (var subsystem in phase.GetSubsystems())
             {
                 subsystem.TearDown(pipelineResult);
             }
         }
     }
-
-    protected virtual void ExecuteSubsystems(PipelineResult pipelineResult)
-    {
-        // TODO: Consider redesign where pipelineResult is not modifiable. 
-        // 
-        foreach (var subsystem in Subsystems)
-        {
-            if (subsystem is not null)
-            {
-                subsystem.ExecuteIfNeeded(pipelineResult);
-            }
-        }
-    }
-
-    protected static void ExecuteIfNeeded(CliSubsystem? subsystem, PipelineResult pipelineResult)
-    {
-        if (subsystem is not null && (!pipelineResult.AlreadyHandled || subsystem.RunsEvenIfAlreadyHandled))
-        {
-            subsystem.ExecuteIfNeeded(pipelineResult);
-        }
-    }
-
 }

src/System.CommandLine.Subsystems/Subsystems/PipelineResult.cs renamed to src/System.CommandLine.Subsystems/PipelineResult.cs

@@ -1,9 +1,7 @@
 // Copyright (c) .NET Foundation and contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
-
-
-namespace System.CommandLine.Subsystems;
+namespace System.CommandLine;
 
 public class PipelineResult(ParseResult? parseResult, string rawInput, Pipeline? pipeline, ConsoleHack? consoleHack = null)
 {