Merge pull request #2472 from KathleenDollard/m-depends-clidatasymbol-pipelineerrors-valueprovider

Changed ValueSubsystem to ValueProvider

Author: KathleenDollard

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

@@ -5,30 +5,63 @@
 using System.CommandLine.Directives;
 using System.CommandLine.Parsing;
 using Xunit;
+using static System.CommandLine.Subsystems.Tests.TestData;
 
 namespace System.CommandLine.Subsystems.Tests;
 
 public class ValueSubsystemTests
 {
     [Fact]
-    public void ValueSubsystem_is_activated_by_default()
+    public void Values_that_are_entered_are_retrieved()
     {
-        CliRootCommand rootCommand = [
-            new CliCommand("x")
-            {
-                new CliOption<string>("--opt1")
-            }];
+        var option = new CliOption<int>("--intOpt");
+        var rootCommand = new CliRootCommand { option };
         var configuration = new CliConfiguration(rootCommand);
-        var subsystem = new ValueSubsystem();
-        var input = "x --opt1 Kirk";
-        var args = CliParser.SplitCommandLine(input).ToList();
+        var pipeline = Pipeline.Create();
+        var consoleHack = new ConsoleHack().RedirectToBuffer(true);
+        var input = "--intOpt 42";
 
-        Subsystem.Initialize(subsystem, configuration, args);
-        var parseResult = CliParser.Parse(rootCommand, args[0], configuration);
-        var isActive = Subsystem.GetIsActivated(subsystem, parseResult);
+        var parseResult = CliParser.Parse(rootCommand, input, configuration);
+        var pipelineResult = new PipelineResult(parseResult, input, pipeline);
 
-        isActive.Should().BeTrue();
+        pipelineResult.Should().NotBeNull();
+        var optionValueResult = pipelineResult.GetValueResult(option);
+        var optionValue = pipelineResult.GetValue<int>(option);
+        optionValueResult.Should().NotBeNull();
+        optionValue.Should().Be(42);
     }
+
+    [Fact]
+    public void Values_that_are_not_entered_are_type_default_with_no_default_values()
+    {
+        var stringOption = new CliOption<string>("--stringOption");
+        var intOption = new CliOption<int>("--intOption");
+        var dateOption = new CliOption<DateTime>("--dateOption");
+        var nullableIntOption = new CliOption<int?>("--nullableIntOption");
+        var guidOption = new CliOption<Guid>("--guidOption");
+        var rootCommand = new CliRootCommand { stringOption, intOption, dateOption, nullableIntOption, guidOption };
+        var configuration = new CliConfiguration(rootCommand);
+        var pipeline = Pipeline.Create();
+        var input = "";
+
+        var parseResult = CliParser.Parse(rootCommand, input, configuration);
+        var pipelineResult = new PipelineResult(parseResult, input, pipeline);
+
+        pipelineResult.Should().NotBeNull();
+        var stringOptionValue = pipelineResult.GetValue<string>(stringOption);
+        var intOptionValue = pipelineResult.GetValue<int>(intOption);
+        var dateOptionValue = pipelineResult.GetValue<DateTime>(dateOption);
+        var nullableIntOptionValue = pipelineResult.GetValue<int?>(nullableIntOption);
+        var guidOptionValue = pipelineResult.GetValue<Guid>(guidOption);
+        stringOptionValue.Should().BeNull();
+        intOptionValue.Should().Be(0);
+        dateOptionValue.Should().Be(DateTime.MinValue);
+        nullableIntOptionValue.Should().BeNull();
+        guidOptionValue.Should().Be(Guid.Empty);
+    }
+
+    // TODO: Add various default value tests
+
     /* Hold these tests until we determine if ValueSubsystem is replaceable
     [Fact]
     public void ValueSubsystem_returns_values_that_are_entered()

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

@@ -67,17 +67,16 @@ private static void Diagram(
         {
             builder.Append('!');
         }
-    */
-    // TODO: Directives
-    /*
+        */
+
+    /* Directives
         switch (symbolResult)
         {
             case DirectiveResult { Directive: not DiagramDirective }:
                 break;
             */
 
-            // 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
-            /*
+    /* 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
             case ArgumentResult argumentResult:
                 {
                     var includeArgumentName =

src/System.CommandLine.Subsystems/Pipeline.cs

@@ -28,7 +28,7 @@ public partial class Pipeline
     /// <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.
+    /// The <see cref="ValueProvider"/>, <see cref="Directives.ResponseSubystem"/>, <see cref="InvocationSubsystem"/>, and <see cref="ValidationSubsystem"/> cannot be replaced.
     /// </remarks>
     public static Pipeline Create(HelpSubsystem? help = null,
                                   VersionSubsystem? version = null,
@@ -56,7 +56,6 @@ public static Pipeline CreateEmpty()
 
     private Pipeline()
     {
-        Value = new ValueSubsystem();
         Response = new ResponseSubsystem();
         Invocation = new InvocationSubsystem();
         Validation = new ValidationSubsystem();
@@ -172,21 +171,16 @@ public ErrorReportingSubsystem? ErrorReporting
 
     // TODO: Consider whether replacing the validation subsystem is valuable
     /// <summary>
-    /// Sets or gets the validation subsystem
+    /// 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
+    /// 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>

src/System.CommandLine.Subsystems/PipelineResult.cs

@@ -5,17 +5,31 @@
 
 namespace System.CommandLine;
 
-public class PipelineResult(ParseResult? parseResult, string rawInput, Pipeline? pipeline, ConsoleHack? consoleHack = null)
+public class PipelineResult(ParseResult parseResult, string rawInput, Pipeline? pipeline, ConsoleHack? consoleHack = null)
 {
+    // TODO: Try to build workflow so it is illegal to create this without a ParseResult
     private readonly List<ParseError> errors = [];
-    public ParseResult? ParseResult { get; } = parseResult;
+    public ParseResult ParseResult { get; } = parseResult;
+    private ValueProvider valueProvider { get; } = new ValueProvider(parseResult);
     public string RawInput { get; } = rawInput;
+
+    // TODO: Consider behavior when pipeline is null - this is probably a core user accessing some subsystems
     public Pipeline Pipeline { get; } = pipeline ?? Pipeline.CreateEmpty();
     public ConsoleHack ConsoleHack { get; } = consoleHack ?? new ConsoleHack();
 
     public bool AlreadyHandled { get; set; }
     public int ExitCode { get; set; }
 
+    public T? GetValue<T>(CliValueSymbol dataSymbol)
+     => valueProvider.GetValue<T>(dataSymbol);
+
+    public object? GetValue(CliValueSymbol option)
+        => valueProvider.GetValue<object?>(option);
+
+    public CliValueResult? GetValueResult(CliValueSymbol dataSymbol)
+     => ParseResult.GetValueResult(dataSymbol);
+
+
     public void AddErrors(IEnumerable<ParseError> errors)
     {
         if (errors is not null)

src/System.CommandLine.Subsystems/Subsystems/PipelinePhase.cs

@@ -41,7 +41,7 @@ private List<CliSubsystem> CreateAfterIfNeeded()
         return after;
     }
 
-    public IEnumerable<CliSubsystem> GetSubsystems() 
+    public IEnumerable<CliSubsystem> GetSubsystems()
         => [
             .. (before is null ? [] : before),
             .. (CliSubsystem is null ? new List<CliSubsystem> { } : [CliSubsystem]),

src/System.CommandLine.Subsystems/ValueAnnotationExtensions.cs

@@ -39,7 +39,7 @@ public static void SetDefaultValue<TValue>(this CliOption<TValue> option, TValue
     /// <param name="option">The option</param>
     /// <returns>The option's default value annotation if any, otherwise <see langword="null"/></returns>
     /// <remarks>
-    /// This is intended to be called by CLI authors. Subsystems should instead call <see cref="ValueSubsystem.GetValue{T}(CliOption)"/>,
+    /// This is intended to be called by CLI authors. Subsystems should instead call <see cref="PipelineResult.GetValue{T}(CliOption)"/>,
     /// which calculates the actual default value, based on the default value annotation and default value calculation,
     /// whether directly stored on the symbol or from the subsystem's <see cref="IAnnotationProvider"/>.
     /// </remarks>
@@ -84,7 +84,7 @@ public static void SetDefaultValue<TValue>(this CliArgument<TValue> argument, TV
     /// <param name="argument">The argument</param>
     /// <returns>The argument's default value annotation if any, otherwise <see langword="null"/></returns>
     /// <remarks>
-    /// This is intended to be called by CLI authors. Subsystems should instead call <see cref="ValueSubsystem.GetValue{T}(CliArgument)"/>,
+    /// This is intended to be called by CLI authors. Subsystems should instead call <see cref="PipelineResult.GetValue{T}(CliArgument)"/>,
     /// which calculates the actual default value, based on the default value annotation and default value calculation,
     /// whether directly stored on the symbol or from the subsystem's <see cref="IAnnotationProvider"/>.
     /// </remarks>
@@ -128,7 +128,7 @@ public static void SetDefaultValueCalculation<TValue>(this CliOption<TValue> opt
     /// <param name="option">The option</param>
     /// <returns>The option's default value calculation if any, otherwise <see langword="null"/></returns>
     /// <remarks>
-    /// This is intended to be called by CLI authors. Subsystems should instead call <see cref="ValueSubsystem.GetValue{T}(CliOption)"/>,
+    /// This is intended to be called by CLI authors. Subsystems should instead call <see cref="PipelineResult.GetValue{T}(CliOption)"/>,
     /// which calculates the actual default value, based on the default value annotation and default value calculation,
     /// whether directly stored on the symbol or from the subsystem's <see cref="IAnnotationProvider"/>.
     /// </remarks>
@@ -173,7 +173,7 @@ public static void SetDefaultValueCalculation<TValue>(this CliArgument<TValue> a
     /// <param name="argument">The argument</param>
     /// <returns>The argument's default value calculation if any, otherwise <see langword="null"/></returns>
     /// <remarks>
-    /// This is intended to be called by CLI authors. Subsystems should instead call <see cref="ValueSubsystem.GetValue{T}(CliArgument)"/>,
+    /// This is intended to be called by CLI authors. Subsystems should instead call <see cref="PipelineResult.GetValue{T}(CliArgument)"/>,
     /// which calculates the actual default value, based on the default value annotation and default value calculation,
     /// whether directly stored on the symbol or from the subsystem's <see cref="IAnnotationProvider"/>.
     /// </remarks>

src/System.CommandLine.Subsystems/ValueSubsystem.cs renamed to src/System.CommandLine.Subsystems/ValueProvider.cs

@@ -6,36 +6,17 @@
 
 namespace System.CommandLine;
 
-public class ValueSubsystem(IAnnotationProvider? annotationProvider = null)
-    : CliSubsystem(ValueAnnotations.Prefix, SubsystemKind.Value, annotationProvider)
+internal class ValueProvider
 {
     private Dictionary<CliSymbol, object?> cachedValues = [];
     private ParseResult? parseResult = null;
 
-    // It is possible that another subsystems GetIsActivated method will access a value. 
-    // If this is called from a GetIsActivated method of a subsystem in the early termination group, 
-    // it will fail. That is not an expected scenario.
-    /// <inheritdoc cref="CliSubsystem.GetIsActivated"/>
-    /// <remarks>
-    /// Note to inheritors: Call base for all ValueSubsystem methods that you override to ensure correct behavior
-    /// </remarks>
-    protected internal override bool GetIsActivated(ParseResult? parseResult)
+    public ValueProvider(ParseResult parseResult)
     {
         this.parseResult = parseResult;
-        return true;
     }
 
-    /// <inheritdoc cref="CliSubsystem.Execute"/>
-    /// <remarks>
-    /// Note to inheritors: Call base for all ValueSubsystem methods that you override to ensure correct behavior
-    /// </remarks>
-    protected internal override void Execute(PipelineResult pipelineResult)
-    {
-        parseResult ??= pipelineResult.ParseResult;
-        base.Execute(pipelineResult);
-    }
-
-    private void SetValue<T>(CliSymbol symbol, object? value)
+    private void SetValue(CliSymbol symbol, object? value)
     {
         cachedValues[symbol] = value;
     }
@@ -53,10 +34,8 @@ private bool TryGetValue<T>(CliSymbol symbol, out T? value)
         return false;
     }
 
-    public T? GetValue<T>(CliOption option)
-        => GetValueInternal<T>(option);
-    public T? GetValue<T>(CliArgument argument)
-        => GetValueInternal<T>(argument);
+    public T? GetValue<T>(CliValueSymbol valueSymbol)
+        => GetValueInternal<T>(valueSymbol);
 
     private T? GetValueInternal<T>(CliSymbol? symbol)
     {
@@ -74,17 +53,17 @@ not null when TryGetValue<T>(symbol, out var value)
             // configuration values go here in precedence
             //not null when GetDefaultFromEnvironmentVariable<T>(symbol, out var envName)
             //    => UseValue(symbol, GetEnvByName(envName)),
-            not null when TryGetAnnotation(symbol, ValueAnnotations.DefaultValueCalculation, out Func<T?>? defaultValueCalculation)
+            not null when symbol.TryGetAnnotation(ValueAnnotations.DefaultValueCalculation, out Func<T?>? defaultValueCalculation)
                 => UseValue(symbol, CalculatedDefault<T>(symbol, (Func<T?>)defaultValueCalculation)),
-            not null when TryGetAnnotation(symbol, ValueAnnotations.DefaultValue, out T? explicitValue)
+            not null when symbol.TryGetAnnotation(ValueAnnotations.DefaultValue, out T? explicitValue)
                 => UseValue(symbol, explicitValue),
             null => throw new ArgumentNullException(nameof(symbol)),
             _ => UseValue(symbol, default(T))
         };
 
         TValue? UseValue<TValue>(CliSymbol symbol, TValue? value)
         {
-            SetValue<TValue>(symbol, value);
+            SetValue(symbol, value);
             return value;
         }
     }