Respond to review

KathleenDollard · KathleenDollard · commit f14d7d02d05b · 2024-08-31T09:09:02.000-04:00
diff --git a/src/System.CommandLine.Subsystems/Subsystems/Annotations/ValueAnnotations.cs b/src/System.CommandLine.Subsystems/Subsystems/Annotations/ValueAnnotations.cs
@@ -4,7 +4,7 @@
 namespace System.CommandLine.Subsystems.Annotations;
 
 /// <summary>
-/// IDs for well-known Version annotations.
+/// IDs for well-known Default Value annotations.
 /// </summary>
 public static class ValueAnnotations
 {
@@ -16,7 +16,7 @@ public static class ValueAnnotations
     public static AnnotationId DefaultValueSource { get; } = new(Prefix, nameof(DefaultValueSource));
 
     /// <summary>
-    /// Default value for an option or argument
+    /// Default default value for an option or argument
     /// </summary>
     /// <remarks>
     /// Should be the same type as the type parameter of
@@ -26,7 +26,7 @@ public static class ValueAnnotations
 
 
     /// <summary>
-    /// Default value calculation for an option or argument
+    /// Default default value calculation for an option or argument
     /// </summary>
     /// <remarks>
     /// Please use the extension methods and do not call this directly.
diff --git a/src/System.CommandLine.Subsystems/Subsystems/Annotations/ValueConditionAnnotations.cs b/src/System.CommandLine.Subsystems/Subsystems/Annotations/ValueConditionAnnotations.cs
@@ -4,7 +4,7 @@
 namespace System.CommandLine.Subsystems.Annotations;
 
 /// <summary>
-/// IDs for well-known Version annotations.
+/// IDs for well-known Value Condition annotations.
 /// </summary>
 public static class ValueConditionAnnotations
 {
diff --git a/src/System.CommandLine.Subsystems/Subsystems/CliSubsystem.cs b/src/System.CommandLine.Subsystems/Subsystems/CliSubsystem.cs
@@ -108,6 +108,7 @@ protected internal bool TryGetAnnotation(CliSymbol symbol, AnnotationId annotati
     /// </summary>
     /// <param name="pipelineResult">The context contains data like the ParseResult, and allows setting of values like whether execution was handled and the CLI should terminate </param>
     /// <returns>A PipelineResult object with information such as whether the CLI should terminate</returns>
+    // These methods are public to support use of subsystems without the pipeline
     public virtual void Execute(PipelineResult pipelineResult)
         => pipelineResult.NotRun(pipelineResult.ParseResult);
 
diff --git a/src/System.CommandLine.Subsystems/Validation/CommandValidator.cs b/src/System.CommandLine.Subsystems/Validation/CommandValidator.cs
@@ -5,11 +5,20 @@
 
 namespace System.CommandLine.Validation;
 
+/// <summary>
+/// Base class for validators that affect the entire command
+/// </summary>
 public abstract class CommandValidator : Validator
 {
     protected CommandValidator(string name, Type valueConditionType, params Type[] moreValueConditionTypes)
         : base(name, valueConditionType, moreValueConditionTypes)
     {  }
 
+    /// <summary>
+    /// Validation method specific to command results.
+    /// </summary>
+    /// <param name="commandResult">The <see cref="CliCommandResult" that will be validated./></param>
+    /// <param name="commandCondition">The <see cref="CommandCondition" that defines the condition that may be validatable./></param>
+    /// <param name="validationContext">The <see cref="ValidationContext" containing information about the current context./></param>
     public abstract void Validate(CliCommandResult commandResult, CommandCondition commandCondition, ValidationContext validationContext);
 }
diff --git a/src/System.CommandLine.Subsystems/Validation/ICommandValidator.cs b/src/System.CommandLine.Subsystems/Validation/ICommandValidator.cs
@@ -5,9 +5,18 @@
 
 namespace System.CommandLine.Validation;
 
+/// <summary>
+/// Interface that allows non-Validator derived methods to perform validation. Specifically, this supports
+/// <see cref="CommandCondition"/> instances that can validate.
+/// </summary>
 public interface ICommandValidator
 {
-
+    /// <summary>
+    /// Validation method specific to command results
+    /// </summary>
+    /// <param name="commandResult">The <see cref="CliCommandResult" that will be validated./></param>
+    /// <param name="commandCondition">The <see cref="CommandCondition" that defines the condition that may be validatable./></param>
+    /// <param name="validationContext">The <see cref="ValidationContext" containing information about the current context./></param>
     void Validate(CliCommandResult commandResult, CommandCondition commandCondition, ValidationContext validationContext);
 }
 
diff --git a/src/System.CommandLine.Subsystems/Validation/IValueValidator.cs b/src/System.CommandLine.Subsystems/Validation/IValueValidator.cs
@@ -5,8 +5,21 @@
 
 namespace System.CommandLine.Validation;
 
+/// <summary>
+/// Interface that allows non-Validator derived methods to perform validation. Specifically, this supports
+/// <see cref="CommandCondition"/> instances that can validate.
+/// </summary>
 public interface IValueValidator
 {
+    // Note: We pass both valueSymbol and valueResult, because we may validate symbols where valueResult is null.
+    /// <summary>
+    /// Validation method specific to value results.
+    /// </summary>
+    /// <param name="value">The value to validate.</param>
+    /// <param name="valueSymbol">The <see cref="CliValueSymbol"/> of the value to validate.</param>
+    /// <param name="valueResult">The <see cref="CliValueResult"/> of the value to validate.</param>
+    /// <param name="valueCondition">The <see cref="ValueCondition" that defines the condition that may be validatable.</param>
+    /// <param name="validationContext">The <see cref="ValidationContext" containing information about the current context./></param>
     void Validate(object? value, CliValueSymbol valueSymbol,
         CliValueResult? valueResult, ValueCondition valueCondition, ValidationContext validationContext);
 }
diff --git a/src/System.CommandLine.Subsystems/Validation/InclusiveGroupValidator.cs b/src/System.CommandLine.Subsystems/Validation/InclusiveGroupValidator.cs
@@ -6,11 +6,15 @@
 
 namespace System.CommandLine.Validation;
 
+/// <summary>
+/// Validator that requires that if one member of the group is present, they are all present.
+/// </summary>
 public class InclusiveGroupValidator : CommandValidator
 {
     public InclusiveGroupValidator() : base(nameof(InclusiveGroup), typeof(InclusiveGroup))
     { }
 
+    /// <inheritdoc/>
     public override void Validate(CliCommandResult commandResult,
         CommandCondition valueCondition, ValidationContext validationContext)
     {
diff --git a/src/System.CommandLine.Subsystems/Validation/RangeValidator.cs b/src/System.CommandLine.Subsystems/Validation/RangeValidator.cs
@@ -5,11 +5,15 @@
 
 namespace System.CommandLine.Validation;
 
+/// <summary>
+/// Validates that a value is within the specified bounds.
+/// </summary>
 public class RangeValidator : ValueValidator, IValueValidator
 {
     public RangeValidator() : base(nameof(ValueConditions.Range), typeof(ValueConditions.Range))
     { }
 
+    /// <inheritdoc/>
     public override void Validate(object? value, CliValueSymbol valueSymbol,
         CliValueResult? valueResult, ValueCondition valueCondition, ValidationContext validationContext)
     {
diff --git a/src/System.CommandLine.Subsystems/Validation/ValidationContext.cs b/src/System.CommandLine.Subsystems/Validation/ValidationContext.cs
@@ -3,6 +3,8 @@
 
 namespace System.CommandLine.Validation;
 
+// TODO: Remove this class. All of the things it contains are in the PipelineResult, except the ValidationSubsystem currently
+//       running, if there are multiple. The scenario where that is needed seems unlikely.
 public class ValidationContext
 {
     public ValidationContext(PipelineResult pipelineResult, ValidationSubsystem validationSubsystem)
diff --git a/src/System.CommandLine.Subsystems/Validation/Validator.cs b/src/System.CommandLine.Subsystems/Validation/Validator.cs
@@ -4,6 +4,10 @@
 
 namespace System.CommandLine.Validation;
 
+// TODO: This may be removed if we settle on ValueCondition validation only.
+/// <summary>
+/// Base class for CommandValidator and ValueValidator. 
+/// </summary>
 public abstract class Validator
 {
     public Validator(string name, Type valueConditionType, params Type[] moreValueConditionTypes)
diff --git a/src/System.CommandLine.Subsystems/Validation/ValueValidator.cs b/src/System.CommandLine.Subsystems/Validation/ValueValidator.cs
@@ -5,6 +5,9 @@
 
 namespace System.CommandLine.Validation;
 
+/// <summary>
+/// Base class for validators that affect a single symbol.
+/// </summary>
 public abstract class ValueValidator : Validator
 {
     protected ValueValidator(string name, Type valueConditionType, params Type[] moreValueConditionTypes)
@@ -16,6 +19,15 @@ protected TValue GetValueAsTypeOrThrow<TValue>(object? value)
             ? typedValue
             : throw new InvalidOperationException($"{Name} validation does not apply to this type");
 
+    /// <summary>
+    /// Validation method specific to a single symbols value.results
+    /// </summary>
+    /// <param name="value">The value to validate.</param>
+    /// <param name="valueSymbol">The option or argument being validated.</param>
+    /// <param name="valueResult">The <see cref="CliValueResult" that will be validated./></param>
+    /// <param name="valueCondition">The <see cref="ValueCondition" that defines the condition that may be validatable./></param>
+    /// <param name="validationContext">The <see cref="ValidationContext" containing information about the current context./></param>
     public abstract void Validate(object? value, CliValueSymbol valueSymbol,
         CliValueResult? valueResult, ValueCondition valueCondition, ValidationContext validationContext);
 }
+
diff --git a/src/System.CommandLine.Subsystems/ValueConditions/InclusiveGroup.cs b/src/System.CommandLine.Subsystems/ValueConditions/InclusiveGroup.cs
@@ -3,15 +3,26 @@
 
 namespace System.CommandLine.ValueConditions;
 
+/// <summary>
+/// Describes that a set of options and arguments must all be entered
+/// if one or more are entered.
+/// </summary>
 public class InclusiveGroup : CommandCondition
 {
     private IEnumerable<CliValueSymbol> group = [];
 
+    /// <summary>
+    /// The constructor for InclusiveGroup.
+    /// </summary>
+    /// <param name="group">The group of options and arguments that must all be present, or note be present.</param>
     public InclusiveGroup(IEnumerable<CliValueSymbol> group)
         : base(nameof(InclusiveGroup))
     {
         this.group = group;
     }
 
+    /// <summary>
+    /// The members of the inclusive group.
+    /// </summary>
     public IEnumerable<CliValueSymbol> Members => group.ToList();
 }
diff --git a/src/System.CommandLine.Subsystems/ValueConditions/Range.cs b/src/System.CommandLine.Subsystems/ValueConditions/Range.cs
@@ -7,6 +7,9 @@
 
 namespace System.CommandLine.ValueConditions;
 
+/// <summary>
+/// Declares the range for the option or argument. The non-generic version is used by the <see cref="RangeValidator/>.
+/// </summary>
 public abstract class Range : ValueCondition
 {
     protected Range(Type valueType)
diff --git a/src/System.CommandLine.Subsystems/ValueConditions/RangeBounds.cs b/src/System.CommandLine.Subsystems/ValueConditions/RangeBounds.cs
@@ -3,11 +3,37 @@
 
 namespace System.CommandLine.ValueConditions;
 
+/// <summary>
+/// Specifies how bounds should be treated.
+/// </summary>
+/// <remarks>
+/// If the lower bound is 2 and the upper bound is 4,
+/// and the value is 2, an inclusive lower bound would 
+/// pass, and an exclusive bound would fail. Similarly, a
+/// value of 4 would fail if the upper bound was inclusive, 
+/// and fail if the bound was exclusive. Exclusive bounds are
+/// often used with fractional values, such as double, single and decimal.
+/// </remarks>
 [Flags]
 public enum RangeBounds
 {
+    /// <summary>
+    /// If the value equals the upper or lower bound, it passes.
+    /// </summary>
     Inclusive = 0,
+
+    /// <summary>
+    /// If the value is greater than the lower bound, and less than or equal to the upper bound, it passes.
+    /// </summary>
     ExclusiveLowerBound = 1,
+
+    /// <summary>
+    /// If the value is greater than or equal to the lower bound, and less than the upper bound, it passes.
+    /// </summary>
     ExclusiveUpperBound = 2,
+
+    /// <summary>
+    /// The value passes only if it is greater than the lower bound and less than the upper bound.
+    /// </summary>
     ExclusiveUpperAndLowerBounds = 3,
 }
diff --git a/src/System.CommandLine.Subsystems/ValueProvider.cs b/src/System.CommandLine.Subsystems/ValueProvider.cs
@@ -40,6 +40,7 @@ private bool TryGetValue<T>(CliSymbol symbol, out T? value)
 
     private T? GetValueInternal<T>(CliValueSymbol valueSymbol)
     {
+        // TODO: Add guard to prevent reentrancy for the same symbol via RelativeToSymbol of custom ValueSource
         var _ = valueSymbol ?? throw new ArgumentNullException(nameof(valueSymbol));
         if (TryGetValue<T>(valueSymbol, out var value))
         {
@@ -55,35 +56,13 @@ private bool TryGetValue<T>(CliSymbol symbol, out T? value)
             {
                 throw new InvalidOperationException("Unexpected ValueSource type");
             }
-            if( typedDefaultValueSource.TryGetTypedValue(pipelineResult, out T? defaultValue))
+            if (typedDefaultValueSource.TryGetTypedValue(pipelineResult, out T? defaultValue))
             {
                 return UseValue(valueSymbol, defaultValue);
             }
         }
         return UseValue(valueSymbol, default(T));
 
-        // TODO: The following logic is definitely WRONG. If there is a relative or env variable, it does not continue if it is not found
-        // TODO: Replace this method with an AggregateValueSource
-        // NOTE: We use the subsystem's TryGetAnnotation here instead of the GetDefaultValue etc
-        // extension methods, as the subsystem's TryGetAnnotation respects its annotation provider
-        //return valueSymbol switch
-        //{
-        //    { } when TryGetValue<T>(valueSymbol, out var value)
-        //        => value, // It has already been retrieved at least once
-        //    { } when parseResult?.GetValueResult(valueSymbol) is { } valueResult  // GetValue not used because it  would always return a value
-        //        => UseValue(valueSymbol, valueResult.GetValue<T>()), // Value was supplied during parsing, 
-        //    // Value was not supplied during parsing, determine default now
-        //    // configuration values go here in precedence
-        //    //not null when GetDefaultFromEnvironmentVariable<T>(symbol, out var envName)
-        //    //    => UseValue(symbol, GetEnvByName(envName)),
-        //    { } when valueSymbol.TryGetAnnotation(ValueAnnotations.DefaultValueCalculation, out Func<T?>? defaultValueCalculation)
-        //        => UseValue(valueSymbol, CalculatedDefault<T>(valueSymbol, (Func<T?>)defaultValueCalculation)),
-        //    { } when valueSymbol.TryGetAnnotation(ValueAnnotations.DefaultValue, out T? explicitValue)
-        //        => UseValue(valueSymbol, explicitValue),
-        //    null => throw new ArgumentNullException(nameof(valueSymbol)),
-        //    _ => UseValue(valueSymbol, default(T))
-        //};
-
         TValue UseValue<TValue>(CliSymbol symbol, TValue value)
         {
             SetValue(symbol, value);
diff --git a/src/System.CommandLine.Subsystems/ValueSources/AggregateValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/AggregateValueSource.cs
@@ -3,11 +3,11 @@
 
 namespace System.CommandLine.ValueSources;
 
-public class AggregateValueSource<T> : ValueSource<T>
+public sealed class AggregateValueSource<T> : ValueSource<T>
 {
     private List<ValueSource<T>> valueSources = [];
 
-    public AggregateValueSource(ValueSource<T> firstSource,
+    internal AggregateValueSource(ValueSource<T> firstSource,
                                  ValueSource<T> secondSource,
                                  string? description = null,
                                  params ValueSource<T>[] otherSources)
@@ -45,8 +45,8 @@ internal static int GetPrecedence(ValueSource<T> source)
         return source switch
         {
             SimpleValueSource<T> => 0,
-            CalculatedValueSource<T> => 1,
-            RelativeToSymbolValueSource<T> => 2,
+            RelativeToSymbolValueSource<T> => 1,
+            CalculatedValueSource<T> => 2,
             //RelativeToConfigurationValueSource<T> => 3,
             RelativeToEnvironmentVariableValueSource<T> => 4,
             _ => 5
diff --git a/src/System.CommandLine.Subsystems/ValueSources/CalculatedValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/CalculatedValueSource.cs
@@ -4,10 +4,17 @@
 namespace System.CommandLine.ValueSources;
 
 // Find an example of when this is useful beyond Random and Guid. Is a time lag between building the CLI and validating important (DateTime.Now())
-public class CalculatedValueSource<T>(Func<(bool success, T? value)> calculation, string? description = null)
-    : ValueSource<T>
+public sealed class CalculatedValueSource<T> : ValueSource<T>
 {
-    public override string? Description { get; } = description;
+    private readonly Func<(bool success, T? value)> calculation;
+
+    internal CalculatedValueSource(Func<(bool success, T? value)> calculation, string? description = null)
+    {
+        this.calculation = calculation;
+        Description = description;
+    }
+
+    public override string? Description { get; }
 
     public override bool TryGetTypedValue(PipelineResult pipelineResult, out T? value)
     {
diff --git a/src/System.CommandLine.Subsystems/ValueSources/RelativeToEnvironmentVariableValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/RelativeToEnvironmentVariableValueSource.cs
diff --git a/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs
diff --git a/src/System.CommandLine.Subsystems/ValueSources/SimpleValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/SimpleValueSource.cs
diff --git a/src/System.CommandLine.Subsystems/ValueSources/ValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/ValueSource.cs

Original file line number	Diff line number	Diff line change
`@@ -4,7 +4,7 @@`
`4`	`4`	`namespace System.CommandLine.Subsystems.Annotations;`
`5`	`5`
`6`	`6`	`/// <summary>`
`7`		`-/// IDs for well-known Version annotations.`
	`7`	`+/// IDs for well-known Value Condition annotations.`
`8`	`8`	`/// </summary>`
`9`	`9`	`public static class ValueConditionAnnotations`
`10`	`10`	`{`
Original file line number	Diff line number	Diff line change
`@@ -6,11 +6,15 @@`
`6`	`6`
`7`	`7`	`namespace System.CommandLine.Validation;`
`8`	`8`
	`9`	`+/// <summary>`
	`10`	`+/// Validator that requires that if one member of the group is present, they are all present.`
	`11`	`+/// </summary>`
`9`	`12`	`public class InclusiveGroupValidator : CommandValidator`
`10`	`13`	`{`
`11`	`14`	`public InclusiveGroupValidator() : base(nameof(InclusiveGroup), typeof(InclusiveGroup))`
`12`	`15`	`{ }`
`13`	`16`
	`17`	`+ /// <inheritdoc/>`
`14`	`18`	`public override void Validate(CliCommandResult commandResult,`
`15`	`19`	`CommandCondition valueCondition, ValidationContext validationContext)`
`16`	`20`	`{`
Original file line number	Diff line number	Diff line change
`@@ -5,11 +5,15 @@`
`5`	`5`
`6`	`6`	`namespace System.CommandLine.Validation;`
`7`	`7`
	`8`	`+/// <summary>`
	`9`	`+/// Validates that a value is within the specified bounds.`
	`10`	`+/// </summary>`
`8`	`11`	`public class RangeValidator : ValueValidator, IValueValidator`
`9`	`12`	`{`
`10`	`13`	`public RangeValidator() : base(nameof(ValueConditions.Range), typeof(ValueConditions.Range))`
`11`	`14`	`{ }`
`12`	`15`
	`16`	`+ /// <inheritdoc/>`
`13`	`17`	`public override void Validate(object? value, CliValueSymbol valueSymbol,`
`14`	`18`	`CliValueResult? valueResult, ValueCondition valueCondition, ValidationContext validationContext)`
`15`	`19`	`{`
Original file line number	Diff line number	Diff line change
`@@ -3,6 +3,8 @@`
`3`	`3`
`4`	`4`	`namespace System.CommandLine.Validation;`
`5`	`5`
	`6`	`+// TODO: Remove this class. All of the things it contains are in the PipelineResult, except the ValidationSubsystem currently`
	`7`	`+// running, if there are multiple. The scenario where that is needed seems unlikely.`
`6`	`8`	`public class ValidationContext`
`7`	`9`	`{`
`8`	`10`	`public ValidationContext(PipelineResult pipelineResult, ValidationSubsystem validationSubsystem)`
Original file line number	Diff line number	Diff line change
`@@ -4,6 +4,10 @@`
`4`	`4`
`5`	`5`	`namespace System.CommandLine.Validation;`
`6`	`6`
	`7`	`+// TODO: This may be removed if we settle on ValueCondition validation only.`
	`8`	`+/// <summary>`
	`9`	`+/// Base class for CommandValidator and ValueValidator.`
	`10`	`+/// </summary>`
`7`	`11`	`public abstract class Validator`
`8`	`12`	`{`
`9`	`13`	`public Validator(string name, Type valueConditionType, params Type[] moreValueConditionTypes)`