Address PR feedback

Author: mhutch

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

@@ -143,7 +143,7 @@ public void Values_above_calculated_upper_bound_report_error()
     public void Values_below_relative_lower_bound_report_error()
     {
         var otherOption = new CliOption<int>("-a");
-        var option = GetOptionWithRangeBounds("--intOpt", ValueSource.Create(otherOption, o => (true, (int)o + 1)), 50);
+        var option = GetOptionWithRangeBounds("--intOpt", ValueSource.Create<int>(otherOption, o => (true, o + 1)), 50);
         var command = new CliCommand("cmd") { option, otherOption };
 
         var pipelineResult = ExecutedPipelineResultForCommand(command, "--intOpt 0 -a 0");
@@ -159,7 +159,9 @@ public void Values_below_relative_lower_bound_report_error()
     public void Values_within_relative_range_do_not_report_error()
     {
         var otherOption = new CliOption<int>("-a");
-        var option = GetOptionWithRangeBounds("--intOpt", ValueSource.Create(otherOption, o => (true, (int)o + 1)), ValueSource.Create(otherOption, o => (true, (int)o + 10)));
+        var option = GetOptionWithRangeBounds("--intOpt",
+            ValueSource.Create<int>(otherOption, o => (true, o + 1)),
+            ValueSource.Create<int>(otherOption, o => (true, o + 10)));
         var command = new CliCommand("cmd") { option, otherOption };
 
         var pipelineResult = ExecutedPipelineResultForCommand(command, "--intOpt 11 -a 3");
@@ -172,7 +174,7 @@ public void Values_within_relative_range_do_not_report_error()
     public void Values_above_relative_upper_bound_report_error()
     {
         var otherOption = new CliOption<int>("-a");
-        var option = GetOptionWithRangeBounds("--intOpt", 0, ValueSource.Create(otherOption, o => (true, (int)o + 10)));
+        var option = GetOptionWithRangeBounds("--intOpt", 0, ValueSource.Create<int>(otherOption, o => (true, o + 10)));
         var command = new CliCommand("cmd") { option, otherOption };
 
         var pipelineResult = ExecutedPipelineResultForCommand(command, "--intOpt 9 -a -2");

src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolver.cs

@@ -41,8 +41,8 @@ public AnnotationResolver(PipelineResult pipelineResult)
     /// <returns>True if successful</returns>
     /// <remarks>
     /// This is intended for use by developers defining custom annotation IDs. Anyone defining an annotation
-    /// ID should also define an accessor extension method on <see cref="AnnotationResolver"/> extension method
-    /// on <see cref="CliSymbol"/> that subsystem authors can use to access the annotation value, such as
+    /// ID should also define an accessor extension method on <see cref="AnnotationResolver"/>
+    /// that subsystem authors can use to access the annotation value, such as
     /// <see cref="HelpAnnotationExtensions.GetDescription{TSymbol}(AnnotationResolver, TSymbol)"/> .
     /// <para>
     /// If the annotation value does not have a single expected type for this symbol, use the

src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.cs

@@ -103,7 +103,7 @@ public static TSymbol WithAnnotation<TSymbol>(this TSymbol symbol, AnnotationId
     /// This is intended to be called by the implementation of specialized ID-specific accessors for CLI authors such as <see cref="HelpAnnotationExtensions.GetDescription{TSymbol}(TSymbol)"/>.
     /// <para>
     /// Subsystems should not call this directly, as it does not account for values from the pipeline's <see cref="IAnnotationProvider"/>.
-    /// They should instead access annotations from the see cref="Pipeline.Annotations"/> property using
+    /// They should instead access annotations from the <see cref="Pipeline.Annotations"/> property using
     /// <see cref="AnnotationResolver.TryGet{TValue}(CliSymbol, AnnotationId, out TValue?)"/> or an ID-specific
     /// extension method such as <see cref="HelpAnnotationExtensions.GetDescription{TSymbol}(TSymbol)" />.
     /// </para>

src/System.CommandLine.Subsystems/ValidationSubsystem.cs

@@ -70,10 +70,11 @@ private void ValidateValue(CliValueSymbol valueSymbol, ValidationContext validat
     {
         var valueConditions = valueSymbol.EnumerateValueConditions();
 
+        // manually implement the foreach so we can efficiently skip getting the
+        // value if there are no conditions
         var enumerator = valueConditions.GetEnumerator();
         if (!enumerator.MoveNext())
         {
-            // avoid getting the value if there are no conditions
             return;
         }
 
@@ -89,6 +90,9 @@ private void ValidateCommand(CliCommandResult commandResult, ValidationContext v
     {
         var valueConditions = commandResult.Command.EnumerateCommandConditions();
 
+        // unlike ValidateValue we do not need to manually implement the foreach
+        // to skip unnecessary work, as there is no additional work to be before
+        // calling command validators
         foreach (var condition in valueConditions)
         {
             ValidateCommandCondition(commandResult, condition, validationContext);

src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs

@@ -20,8 +20,7 @@ public static class ValueConditionAnnotationExtensions
     /// <param name="symbol">The option or argument the range applies to.</param>
     /// <param name="lowerBound">The lower bound of the range.</param>
     /// <param name="upperBound">The upper bound of the range.</param>
-    // TODO: Add RangeBounds
-    // TODO: You should not have to set both...why not nullable?
+    // TODO: can we eliminate this overload and just reply on the implicit cast to ValueSource<TValue>?
     public static void SetRange<TValueSymbol, TValue>(this TValueSymbol symbol, TValue lowerBound, TValue upperBound)
         where TValueSymbol : CliValueSymbol, ICliValueSymbol<TValue>
         where TValue : IComparable<TValue>
@@ -40,12 +39,9 @@ public static void SetRange<TValueSymbol, TValue>(this TValueSymbol symbol, TVal
     /// <param name="symbol">The option or argument the range applies to.</param>
     /// <param name="lowerBound">The <see cref="ValueSource"> that is the lower bound of the range.</param>
     /// <param name="upperBound">The <see cref="ValueSource"> that is the upper bound of the range.</param>
-    // TODO: Add RangeBounds
-    // TODO: You should not have to set both...why not nullable?
-    public static void SetRange<TValueSymbol, TValue>(this TValueSymbol symbol, ValueSource<TValue> lowerBound, ValueSource<TValue> upperBound)
+    public static void SetRange<TValueSymbol, TValue>(this TValueSymbol symbol, ValueSource<TValue>? lowerBound, ValueSource<TValue>? upperBound)
         where TValueSymbol : CliValueSymbol, ICliValueSymbol<TValue>
         where TValue : IComparable<TValue>
-        // TODO: You should not have to set both...why not nullable?
     {
         var range = new Range<TValue>(lowerBound, upperBound);
 
@@ -68,13 +64,11 @@ public static void SetRange<TValueSymbol, TValue>(this TValueSymbol symbol, Valu
     public static void SetInclusiveGroup(this CliCommand command, IEnumerable<CliValueSymbol> group)
         => command.SetValueCondition(new InclusiveGroup(group));
 
-    // TODO: This should not be public if ValueConditions are not public
     public static void SetValueCondition<TValueSymbol, TValueCondition>(this TValueSymbol symbol, TValueCondition valueCondition)
         where TValueSymbol : CliValueSymbol
         where TValueCondition : ValueCondition
         => symbol.AddAnnotation(ValueConditionAnnotations.ValueConditions, valueCondition);
 
-    // TODO: This should not be public if ValueConditions are not public
     public static void SetValueCondition<TCommandCondition>(this CliCommand symbol, TCommandCondition commandCondition)
         where TCommandCondition : CommandCondition
         => symbol.AddAnnotation(ValueConditionAnnotations.ValueConditions, commandCondition);
@@ -84,8 +78,6 @@ public static void SetValueCondition<TCommandCondition>(this CliCommand symbol,
     /// </summary>
     /// <param name="command">The option or argument to get the conditions for.</param>
     /// <returns>The conditions that have been applied to the option or argument.</returns>
-    ///
-    // TODO: This is public because it will be used by other subsystems we might not own. It could be an extension method the subsystem namespace
     public static IEnumerable<ValueCondition> EnumerateValueConditions(this CliValueSymbol symbol)
         => symbol.EnumerateAnnotations<ValueCondition>(ValueConditionAnnotations.ValueConditions);
 
@@ -94,8 +86,6 @@ public static IEnumerable<ValueCondition> EnumerateValueConditions(this CliValue
     /// </summary>
     /// <param name="command">The option or argument to get the conditions for.</param>
     /// <returns>The conditions that have been applied to the option or argument.</returns>
-    ///
-    // TODO: This is public because it will be used by other subsystems we might not own. It could be an extension method the subsystem namespace
     public static IEnumerable<ValueCondition> EnumerateValueConditions(this AnnotationResolver resolver, CliValueSymbol symbol)
         => resolver.Enumerate<ValueCondition>(symbol, ValueConditionAnnotations.ValueConditions);
 
@@ -104,8 +94,6 @@ public static IEnumerable<ValueCondition> EnumerateValueConditions(this Annotati
     /// </summary>
     /// <param name="command">The command to get the conditions for.</param>
     /// <returns>The conditions that have been applied to the command.</returns>
-    ///
-    // TODO: This is public because it will be used by other subsystems we might not own. It could be an extension method the subsystem namespace
     public static IEnumerable<CommandCondition> EnumerateCommandConditions(this CliCommand command)
         => command.EnumerateAnnotations<CommandCondition>(ValueConditionAnnotations.ValueConditions);
 
@@ -125,19 +113,16 @@ public static IEnumerable<CommandCondition> EnumerateCommandConditions(this Anno
     /// <typeparam name="TCondition">The type of condition to return.</typeparam>
     /// <param name="symbol">The option or argument that may contain the condition.</param>
     /// <returns>The condition if it exists on the option or argument, otherwise null.</returns>
-    // This method feels useful because it clarifies that last should win and returns one, when only one should be applied
-    // TODO: Consider removing user facing naming, other than the base type, that is Value or CommandCondition and just use Condition
     public static TCondition? GetValueCondition<TCondition>(this CliValueSymbol symbol)
         where TCondition : ValueCondition
-        => symbol.EnumerateValueConditions().OfType<TCondition>().LastOrDefault();
+        => symbol.EnumerateValueConditions().OfType<TCondition>().FirstOrDefault();
 
     /// <summary>
     /// Gets the condition that matches the type, if it exists on this command.
     /// </summary>
     /// <typeparam name="TCondition">The type of condition to return.</typeparam>
     /// <param name="symbol">The command that may contain the condition.</param>
     /// <returns>The condition if it exists on the command, otherwise null.</returns>
-    // This method feels useful because it clarifies that last should win and returns one, when only one should be applied
     public static TCondition? GetCommandCondition<TCondition>(this CliCommand symbol)
         where TCondition : CommandCondition
         => symbol.EnumerateCommandConditions().OfType<TCondition>().FirstOrDefault();

src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs

@@ -17,7 +17,7 @@ public sealed class RelativeToSymbolValueSource<T>
     internal RelativeToSymbolValueSource(
        CliValueSymbol otherSymbol,
        bool onlyUserEnteredValues = false,
-       Func<object?, (bool success, T? value)>? calculation = null,
+       Func<T?, (bool success, T? value)>? calculation = null,
        string? description = null)
     {
         OtherSymbol = otherSymbol;
@@ -29,7 +29,7 @@ internal RelativeToSymbolValueSource(
     public override string? Description { get; }
     public CliValueSymbol OtherSymbol { get; }
     public bool OnlyUserEnteredValues { get; }
-    public Func<object?, (bool success, T? value)>? Calculation { get; }
+    public Func<T?, (bool success, T? value)>? Calculation { get; }
 
     /// <inheritdoc/>
     public override bool TryGetTypedValue(PipelineResult pipelineResult, out T? value)

src/System.CommandLine.Subsystems/ValueSources/ValueSource.cs

@@ -31,7 +31,7 @@ public static ValueSource<T> Create<T>(Func<(bool success, T? value)> calculatio
         => new CalculatedValueSource<T>(calculation, description);
 
     public static ValueSource<T> Create<T>(CliValueSymbol otherSymbol,
-                                           Func<object?, (bool success, T? value)>? calculation = null,
+                                           Func<T?, (bool success, T? value)>? calculation = null,
                                            bool userEnteredValueOnly = false,
                                            string? description = null)
         => new RelativeToSymbolValueSource<T>(otherSymbol, userEnteredValueOnly, calculation, description);

src/System.CommandLine/ICliValueSymbol.cs

@@ -4,7 +4,7 @@
 namespace System.CommandLine;
 
 /// <summary>
-/// This is applied to <see cref="CliArgument{T}"/> and <see cref="CliArgument{T}"/>, and allows
+/// This is implemented only by <see cref="CliOption{T}"/> and <see cref="CliArgument{T}"/>, and allows
 /// allows methods with a <see cref="CliValueSymbol"/> argument to apply constraints based on the
 /// value type.
 /// </summary>