Add XML docs, adjust scope, remove some unused stuff

Author: KathleenDollard

OpenQuestions.md

@@ -54,3 +54,6 @@ However, the point of conditions is that they are a statement about the symbol a
 
 Suggestion: Use internal constructors and leave conditions public
 
+## Should `ValueCondition` be called `Condition`?
+
+They may apply to commands.

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>
-    /// The <see cref="ValueProvider"/>, <see cref="Directives.ResponseSubystem"/>, <see cref="InvocationSubsystem"/>, and <see cref="ValidationSubsystem"/> cannot be replaced.
+    /// The <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,

src/System.CommandLine.Subsystems/PipelineResult.cs

@@ -30,14 +30,14 @@ public PipelineResult(ParseResult parseResult, string rawInput, Pipeline? pipeli
     public bool AlreadyHandled { get; set; }
     public int ExitCode { get; set; }
 
-    public T? GetValue<T>(CliValueSymbol dataSymbol)
-     => valueProvider.GetValue<T>(dataSymbol);
+    public T? GetValue<T>(CliValueSymbol valueSymbol)
+     => valueProvider.GetValue<T>(valueSymbol);
 
     public object? GetValue(CliValueSymbol option)
         => valueProvider.GetValue<object>(option);
 
-    public CliValueResult? GetValueResult(CliValueSymbol dataSymbol)
-     => ParseResult.GetValueResult(dataSymbol);
+    public CliValueResult? GetValueResult(CliValueSymbol valueSymbol)
+     => ParseResult.GetValueResult(valueSymbol);
 
 
     public void AddErrors(IEnumerable<ParseError> errors)

src/System.CommandLine.Subsystems/Validation/InclusiveGroupValidator.cs

@@ -44,7 +44,7 @@ public override void Validate(CliCommandResult commandResult,
             // TODO: Rework to allow localization
             var pluralToBe = "are";
             var singularToBe = "is";
-            validationContext.PipelineResult.AddError(new ParseError( $"The members {string.Join(", ", groupMembers.Select(m => m.Name))} " +
+            validationContext.AddError(new ParseError( $"The members {string.Join(", ", groupMembers.Select(m => m.Name))} " +
                 $"must all be used if one is used. {string.Join(", ", missingMembers.Select(m => m.Name))} " +
                 $"{(missingMembers.Skip(1).Any() ? pluralToBe : singularToBe)} missing."));
         }

src/System.CommandLine.Subsystems/Validation/RangeValidator.cs

@@ -24,7 +24,7 @@ public override void Validate(object? value, CliValueSymbol valueSymbol,
         }
         if (valueCondition.MustHaveValidator)
         {
-            validationContext.PipelineResult.AddError(new ParseError($"Range validator missing for {valueSymbol.Name}"));
+            validationContext.AddError(new ParseError($"Range validator missing for {valueSymbol.Name}"));
         }
     }
 

src/System.CommandLine.Subsystems/Validation/ValidationContext.cs

@@ -1,20 +1,57 @@
 // 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.Parsing;
+using System.CommandLine.ValueSources;
+using static System.Runtime.InteropServices.JavaScript.JSType;
+
 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.
+/// <summary>
+/// Provides the context for IValidator implementations
+/// </summary>
 public class ValidationContext
 {
-    public ValidationContext(PipelineResult pipelineResult, ValidationSubsystem validationSubsystem)
+    private PipelineResult pipelineResult { get; }
+
+    internal ValidationContext(PipelineResult pipelineResult, ValidationSubsystem validationSubsystem)
     {
-        PipelineResult = pipelineResult;
+        this.pipelineResult = pipelineResult;
         ValidationSubsystem = validationSubsystem;
     }
 
-    public PipelineResult PipelineResult { get; }
-    public Pipeline Pipeline => PipelineResult.Pipeline;
-    public ValidationSubsystem ValidationSubsystem { get; }
-    public ParseResult? ParseResult => PipelineResult.ParseResult;
+    /// <summary>
+    /// Adds an error to the PipelineContext.
+    /// </summary>
+    /// <param name="error">The <see cref="ParseError"/> to add</param>
+    public void AddError(ParseError error)
+        => pipelineResult.AddError(error);
+
+    /// <summary>
+    /// Gets the value for an option or argument.
+    /// </summary>
+    /// <param name="valueSymbol">The symbol to get the value for.</param>
+    /// <returns></returns>
+    public object? GetValue(CliValueSymbol valueSymbol)
+        => pipelineResult.GetValue<object>(valueSymbol);
+
+    /// <summary>
+    /// Gets the <see cref="ValueResult"/> for the option or argument, if the user entered a value. 
+    /// </summary>
+    /// <param name="valueSymbol">The symbol to get the ValueResult for.</param>
+    /// <returns>The ValueResult for the option or argument, or null if the user did not enter a value.</returns>
+    public CliValueResult? GetValueResult(CliValueSymbol valueSymbol)
+        => pipelineResult.GetValueResult(valueSymbol);
+
+    /// <summary>
+    /// Tries to get the value for a <see cref="ValueSource"/> and returns it a an `out` parameter.
+    /// </summary>
+    /// <typeparam name="T">The type of the value to retrieve</typeparam>
+    /// <param name="valueSource">The <see cref="ValueSource"/> to query for its result.</param>
+    /// <param name="value">An output parameter that contains the value, if it is found.</param>
+    /// <returns>True if the <see cref="ValueSource"/> succeeded, otherwise false.</returns>
+    public bool TryGetTypedValue<T>(ValueSource<T> valueSource, out T? value)
+        => valueSource.TryGetTypedValue(pipelineResult, out value);
+
+    internal ValidationSubsystem ValidationSubsystem { get; }
 }

src/System.CommandLine.Subsystems/ValidationSubsystem.cs

@@ -74,8 +74,8 @@ private void ValidateValue(CliValueSymbol valueSymbol, ValidationContext validat
             return; // nothing to do
         }
 
-        var value = validationContext.PipelineResult.GetValue(valueSymbol);
-        var valueResult = validationContext.ParseResult?.GetValueResult(valueSymbol);
+        var value = validationContext.GetValue(valueSymbol);
+        var valueResult = validationContext.GetValueResult(valueSymbol);
         foreach (var condition in valueConditions)
         {
             ValidateValueCondition(value, valueSymbol, valueResult, condition, validationContext);
@@ -120,7 +120,7 @@ private void ValidateValueCondition(object? value, CliValueSymbol valueSymbol, C
         {
             if (condition.MustHaveValidator)
             {
-                validationContext.PipelineResult.AddError(new ParseError($"{valueSymbol.Name} must have {condition.Name} validator."));
+                validationContext.AddError(new ParseError($"{valueSymbol.Name} must have {condition.Name} validator."));
             }
             return; 
         }
@@ -166,7 +166,7 @@ private void ValidateCommandCondition(CliCommandResult commandResult, CommandCon
         {
             if (condition.MustHaveValidator)
             {
-                validationContext.PipelineResult.AddError(new ParseError($"{commandResult.Command.Name} must have {condition.Name} validator."));
+                validationContext.AddError(new ParseError($"{commandResult.Command.Name} must have {condition.Name} validator."));
             }
             return;
         }

src/System.CommandLine.Subsystems/ValueCondition.cs

@@ -3,8 +3,22 @@
 
 namespace System.CommandLine;
 
+/// <summary>
+/// The base class for all conditions. Conditions describe aspects of 
+/// symbol results, including restrictions used for validation. 
+/// </summary>
+/// <param name="name"></param>
 public abstract class ValueCondition(string name)
 {
+    /// <summary>
+    /// Whether a diagnostic should be reported if there is no validator. 
+    /// Conditions may be used for other purposes, such as completions and 
+    /// not require validation.
+    /// </summary>
     public virtual bool MustHaveValidator { get; } = true;
+
+    /// <summary>
+    /// The name of the ValueCondition.
+    /// </summary>
     public string Name { get; } = name;
 }

src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs

@@ -4,8 +4,20 @@
 
 namespace System.CommandLine;
 
+/// <summary>
+/// Contains the extension methods that are used to create value conditions
+/// </summary>
 public static class ValueConditionAnnotationExtensions
 {
+    /// <summary>
+    /// Set the upper and/or lower bound values of the range.
+    /// </summary>
+    /// <typeparam name="T">The type of the bounds.</typeparam>
+    /// <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?
     public static void SetRange<T>(this CliValueSymbol symbol, T lowerBound, T upperBound)
         where T : IComparable<T>
     {
@@ -14,33 +26,35 @@ public static void SetRange<T>(this CliValueSymbol symbol, T lowerBound, T upper
         symbol.SetValueCondition(range);
     }
 
-    public static void SetRange<T>(this CliValueSymbol symbol, ValueSource<T> lowerBound, T upperBound)
-    where T : IComparable<T>
-    {
-        var range = new Range<T>(lowerBound, upperBound);
-
-        symbol.SetValueCondition(range);
-    }
-
-    public static void SetRange<T>(this CliValueSymbol symbol, T lowerBound, ValueSource<T> upperBound)
-    where T : IComparable<T>
-    {
-        var range = new Range<T>(lowerBound, upperBound);
-
-        symbol.SetValueCondition(range);
-    }
-
+    /// <summary>
+    /// Set the upper and/or lower bound via ValueSource. Implicit conversions means this 
+    /// generally just works with any <see cref="ValueSource">.
+    /// </summary>
+    /// <typeparam name="T">The type of the bounds.</typeparam>
+    /// <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<T>(this CliValueSymbol symbol, ValueSource<T> lowerBound, ValueSource<T> upperBound)
-    where T : IComparable<T>
+        where T : IComparable<T>
+        // TODO: You should not have to set both...why not nullable?
     {
         var range = new Range<T>(lowerBound, upperBound);
 
         symbol.SetValueCondition(range);
     }
 
-    public static void SetInclusiveGroup(this CliCommand symbol, IEnumerable<CliValueSymbol> group)
-        => symbol.SetValueCondition(new InclusiveGroup(group));
+    /// <summary>
+    /// Indicates that there is an inclusive group of options and arguments for the command. All
+    /// members of an inclusive must be present, or none can be present.
+    /// </summary>
+    /// <param name="command">The command the inclusive group applies to.</param>
+    /// <param name="group">The group of options and arguments that must all be present, or none can be present.</param>
+    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
@@ -53,33 +67,63 @@ public static void SetValueCondition<TValueSymbol, TValueCondition>(this TValueS
         valueConditions.Add(valueCondition);
     }
 
-    public static void SetValueCondition<TValueCondition>(this CliCommand symbol, TValueCondition valueCondition)
-        where TValueCondition : CommandCondition
+    // TODO: This should not be public if ValueConditions are not public
+    public static void SetValueCondition<TCommandCondition>(this CliCommand symbol, TCommandCondition commandCondition)
+        where TCommandCondition : CommandCondition
     {
         if (!symbol.TryGetAnnotation<List<CommandCondition>>(ValueConditionAnnotations.ValueConditions, out var valueConditions))
         {
             valueConditions = [];
             symbol.SetAnnotation(ValueConditionAnnotations.ValueConditions, valueConditions);
         }
-        valueConditions.Add(valueCondition);
+        valueConditions.Add(commandCondition);
     }
 
+    /// <summary>
+    /// Gets a list of conditions on an option or argument.
+    /// </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 List<ValueCondition>? GetValueConditions(this CliValueSymbol symbol)
         => symbol.TryGetAnnotation<List<ValueCondition>>(ValueConditionAnnotations.ValueConditions, out var valueConditions)
             ? valueConditions
             : null;
 
-    public static List<CommandCondition>? GetCommandConditions(this CliCommand symbol)
-        => symbol.TryGetAnnotation<List<CommandCondition>>(ValueConditionAnnotations.ValueConditions, out var valueConditions)
+    /// <summary>
+    /// Gets a list of conditions on a command.
+    /// </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 List<CommandCondition>? GetCommandConditions(this CliCommand command)
+        => command.TryGetAnnotation<List<CommandCondition>>(ValueConditionAnnotations.ValueConditions, out var valueConditions)
             ? valueConditions
             : null;
 
+    /// <summary>
+    /// Gets the condition that matches the type, if it exists on this option or argument.
+    /// </summary>
+    /// <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.TryGetAnnotation(ValueConditionAnnotations.ValueConditions, out List<ValueCondition>? valueConditions)
             ? null
             : valueConditions.OfType<TCondition>().LastOrDefault();
 
+    /// <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.TryGetAnnotation(ValueConditionAnnotations.ValueConditions, out List<CommandCondition>? valueConditions)