Cleanup and fixes

- CommandValueResult ctor internal
- ValueResult ctor restricted to CliOption and CliArgument
- Added XML comments
- Removed dead code I was confident had been replaced

Author: KathleenDollard

src/System.CommandLine/ParseResult.cs

@@ -82,44 +82,7 @@ internal ParseResult(
             Errors = errors is not null ? errors : Array.Empty<ParseError>();
         }
 
-        //private Dictionary<string, CliSymbol> PopulateSymbolByName()
-        //{
-        //    var commands = GetSelfAndAncestors(CommandResult);
-        //    var ret = new Dictionary<string, CliSymbol> { };
-
-        //    foreach (var command in commands)
-        //    {
-        //        if (command.HasOptions)
-        //        {
-        //            foreach (var option in command.Options)
-        //            {
-        //                ret[option.Name] = option;
-        //            }
-        //        }
-        //        if (command.HasArguments)
-        //        {
-        //            foreach (var argument in command.Arguments)
-        //            {
-        //                ret[argument.Name] = argument;
-        //            }
-        //        }
-        //    }
-        //    return ret;
-
-        //    static IEnumerable<CliCommand> GetSelfAndAncestors(CommandResult commandResult)
-        //    {
-        //        var ret = new List<CliCommand> { commandResult.Command };
-        //        while (commandResult.Parent is CommandResult parent)
-        //        {
-        //            commandResult = parent;
-        //            ret.Add(parent.Command);
-        //        }
-        //        ret.Reverse();
-        //        return ret;
-        //    }
-        //}
-
-        public CliSymbol GetSymbolByName(string name, bool valuesOnly = false)
+        public CliSymbol? GetSymbolByName(string name, bool valuesOnly = false)
         {
 
             symbolLookupByName ??= new SymbolLookupByName(this);
@@ -232,10 +195,22 @@ CommandLineText is null
         public override string ToString() => ParseDiagramAction.Diagram(this).ToString();
         */
 
+        /// <summary>
+        /// Gets the ValueResult, if any, for the specified option.
+        /// </summary>
+        /// <param name="option">The option for which to find a result.</param>
+        /// <returns>A result for the specified option, or <see langword="null"/> if it was not entered by the user.</returns>
         public ValueResult? GetValueResult(CliOption option)
             => GetValueResultInternal(option);
+
+        /// <summary>
+        /// Gets the result, if any, for the specified argument.
+        /// </summary>
+        /// <param name="argument">The argument for which to find a result.</param>
+        /// <returns>A result for the specified argument, or <see langword="default"/> if it was not entered by the user.</returns>
         public ValueResult? GetValueResult(CliArgument argument)
             => GetValueResultInternal(argument);
+
         private ValueResult? GetValueResultInternal(CliSymbol symbol)
             => valueResultDictionary.TryGetValue(symbol, out var result)
                 ? result

src/System.CommandLine/Parsing/CommandValueResult.cs

@@ -5,15 +5,38 @@
 
 namespace System.CommandLine.Parsing;
 
+/// <summary>
+/// Provides the publicly facing command result
+/// </summary>
+/// <remarks>
+/// The name is temporary as we expect to later name this CommandResult and the previous one to CommandResultInternal
+/// </remarks>
 public class CommandValueResult
 {
-    public CommandValueResult(CliCommand command, CommandValueResult parent)
+    /// <summary>
+    /// Creates a CommandValueResult instance
+    /// </summary>
+    /// <param name="command">The CliCommand that the result is for.</param>
+    /// <param name="parent">The parent command in the case of a CLI hierarchy, or null if there is no parent.</param>
+    internal CommandValueResult(CliCommand command, CommandValueResult parent = null)
     {
         Command = command;
         Parent = parent;
     }
+
+    /// <summary>
+    /// The ValueResult instances for user entered data. This is a sparse list.
+    /// </summary>
     public IEnumerable<ValueResult> ValueResults { get; } = new List<ValueResult>();
+
+    /// <summary>
+    /// The CliCommand that the result is for. 
+    /// </summary>
     public CliCommand Command { get; }
-    public CommandValueResult Parent { get; }
+
+    /// <summary>
+    /// The command's parent if one exists, otherwise, null
+    /// </summary>
+    public CommandValueResult? Parent { get; }
 
 }

src/System.CommandLine/Parsing/SymbolLookupByName.cs

@@ -12,9 +12,12 @@ namespace System.CommandLine.Parsing;
 // give correct results for many CLIs, and also, it built a dictionary for the full CLI tree, rather than just the
 // current commands and its ancestors, so in many cases, this will be faster. 
 //
-// Most importantly, that approach fails for options, like the previous global options, that appear on multiple
+// Most importantly, the previous approach fails for options, like the previous global options, that appear on multiple
 // commands, since we are now explicitly putting them on all commands.
 
+/// <summary>
+/// Provides a mechanism to lookup symbols by their name. This searches the symbols corresponding to the current command and its ancestors.
+/// </summary>
 public class SymbolLookupByName
 {
     private class CommandCache(CliCommand command)
@@ -25,6 +28,11 @@ private class CommandCache(CliCommand command)
 
     private List<CommandCache> cache;
 
+    /// <summary>
+    /// Creates a new symbol lookup tied to a specific parseResult. 
+    /// </summary>
+    /// <param name="parseResult"></param>
+    // TODO: If needed, consider a static list/dictionary of ParseResult to make general use easier.
     public SymbolLookupByName(ParseResult parseResult)
         => cache = BuildCache(parseResult);
 
@@ -121,12 +129,31 @@ private bool TryGetSymbolAndParentInternal(string name,
         return false;
     }
 
+    /// <summary>
+    /// Gets the symbol with the requested name that appears nearest to the starting command, which defaults to the current or leaf command.
+    /// </summary>
+    /// <param name="name">The name to search for</param>
+    /// <param name="startCommand">The command to start searching up from, which defaults to the current command.</param>
+    /// <param name="skipAncestors">If true, only the starting command and no ancestors are searched.</param>
+    /// <param name="valuesOnly">If true, commands are ignored and only options and arguments are found.</param>
+    /// <returns>A tuple of the found symbol and its parent command. Throws if the name is not found.</returns>
+    /// <exception cref="InvalidOperationException">Thrown if the name is not found.</exception>
     public (CliSymbol symbol, CliCommand parent) GetSymbolAndParent(string name, CliCommand? startCommand = null, bool skipAncestors = false, bool valuesOnly = false)
         => TryGetSymbolAndParentInternal(name, out var symbol, out var parent, out var errorMessage, startCommand, skipAncestors, valuesOnly)
             ? (symbol, parent)
             : throw new InvalidOperationException(errorMessage);
 
-    public bool TryGetSymbol(string name, out CliSymbol symbol, CliCommand? startCommand = null, bool skipAncestors = false, bool valuesOnly = false)
+
+    /// <summary>
+    /// Returns true if the symbol is found, and provides the symbols as the `out` symbol parameter.
+    /// </summary>
+    /// <param name="name">The name to search for</param>
+    /// <param name="symbol">An out parameter to receive the symbol if it is found.</param>
+    /// <param name="startCommand">The command to start searching up from, which defaults to the current command.</param>
+    /// <param name="skipAncestors">If true, only the starting command and no ancestors are searched.</param>
+    /// <param name="valuesOnly">If true, commands are ignored and only options and arguments are found.</param>
+    /// <returns>True if a symbol with the requested name is found</returns>
+    public bool TryGetSymbol(string name, [NotNullWhen(true)] out CliSymbol? symbol, CliCommand? startCommand = null, bool skipAncestors = false, bool valuesOnly = false)
         => TryGetSymbolAndParentInternal(name, out symbol, out var _, out var _, startCommand, skipAncestors, valuesOnly);
 
 

src/System.CommandLine/Parsing/SymbolResultTree.cs

@@ -18,7 +18,6 @@ internal sealed class SymbolResultTree : Dictionary<CliSymbol, SymbolResult>
         // TODO: Looks like this is a SymboNode/linked list because a symbol may appear multiple
         // places in the tree and multiple symbols will have the same short name. The question is 
         // whether creating the multiple node instances is faster than just using lists. Could well be.
-        //private Dictionary<string, SymbolNode>? _symbolsByName;
         internal SymbolResultTree(
             CliCommand rootCommand,
             List<string>? tokenizeErrors)
@@ -47,11 +46,6 @@ internal SymbolResultTree(
         internal OptionResult? GetResult(CliOption option)
             => TryGetValue(option, out SymbolResult? result) ? (OptionResult)result : default;
 
-        //TODO: directives
-        /* 
-                internal DirectiveResult? GetResult(CliDirective directive)
-                    => TryGetValue(directive, out SymbolResult? result) ? (DirectiveResult)result : default;
-        */
         // TODO: Determine how this is used. It appears to be O^n in the size of the tree and so if it is called multiple times, we should reconsider to avoid O^(N*M)
         internal IEnumerable<SymbolResult> GetChildren(SymbolResult parent)
         {
@@ -109,91 +103,5 @@ internal void AddUnmatchedToken(CliToken token, CommandResult commandResult, Com
             //            }
         }
 
-        /* No longer used
-        public SymbolResult? GetResult(string name)
-        {
-            if (_symbolsByName is null)
-            {
-                _symbolsByName = new();
-                // TODO: See if we can avoid populating the entire tree and just populate the portion/cone we need
-                PopulateSymbolsByName(_rootCommand);
-            }
-
-            if (!_symbolsByName.TryGetValue(name, out SymbolNode? node))
-            {
-                throw new ArgumentException($"No symbol result found with name \"{name}\".");
-            }
-
-            while (node is not null)
-            {
-                if (TryGetValue(node.Symbol, out var result))
-                {
-                    return result;
-                }
-
-                node = node.Next;
-            }
-
-            return null;
-        }
-
-        // TODO: symbolsbyname - this is inefficient
-        // results for some values may not be queried at all, dependent on other options
-        // so we could avoid using their value factories and adding them to the dictionary
-        // could we sort by name allowing us to do a binary search instead of allocating a dictionary?
-        // could we add codepaths that query for specific kinds of symbols so they don't have to search all symbols?
-        // Additional Note: Couldn't commands know their children, and thus this involves querying the active command, and possibly the parents
-        private void PopulateSymbolsByName(CliCommand command)
-        {
-            if (command.HasArguments)
-            {
-                for (var i = 0; i < command.Arguments.Count; i++)
-                {
-                    AddToSymbolsByName(command.Arguments[i]);
-                }
-            }
-
-            if (command.HasOptions)
-            {
-                for (var i = 0; i < command.Options.Count; i++)
-                {
-                    AddToSymbolsByName(command.Options[i]);
-                }
-            }
-
-            if (command.HasSubcommands)
-            {
-                for (var i = 0; i < command.Subcommands.Count; i++)
-                {
-                    var childCommand = command.Subcommands[i];
-                    AddToSymbolsByName(childCommand);
-                    PopulateSymbolsByName(childCommand);
-                }
-            }
-
-            // TODO: Explore removing closure here
-            void AddToSymbolsByName(CliSymbol symbol)
-            {
-                if (_symbolsByName!.TryGetValue(symbol.Name, out var node))
-                {
-                    if (symbol.Name == node.Symbol.Name &&
-                        symbol.FirstParent?.Symbol is { } parent &&
-                        parent == node.Symbol.FirstParent?.Symbol)
-                    {
-                        throw new InvalidOperationException($"Command {parent.Name} has more than one child named \"{symbol.Name}\".");
-                    }
-
-                    _symbolsByName[symbol.Name] = new(symbol)
-                    {
-                        Next = node
-                    };
-                }
-                else
-                {
-                    _symbolsByName[symbol.Name] = new(symbol);
-                }
-            }
-        }
-        */
     }
 }

src/System.CommandLine/Parsing/ValueResult.cs

@@ -5,9 +5,12 @@
 
 namespace System.CommandLine.Parsing;
 
+/// <summary>
+/// The publicly facing class for argument and option data.
+/// </summary>
 public class ValueResult
 {
-    internal ValueResult(
+    private ValueResult(
         CliSymbol valueSymbol,
         object? value,
         IEnumerable<Location> locations,
@@ -22,22 +25,82 @@ internal ValueResult(
         Error = error;
     }
 
+    /// <summary>
+    /// Creates a new ValueResult instance
+    /// </summary>
+    /// <param name="argument">The CliArgument the value is for.</param>
+    /// <param name="value">The entered value.</param>
+    /// <param name="locations">The locations list.</param>
+    /// <param name="outcome">True if parsing and converting the value was successful.</param>
+    /// <param name="error">The CliError if parsing or converting failed, otherwise null.</param>
+    internal ValueResult(
+            CliArgument argument,
+            object? value,
+            IEnumerable<Location> locations,
+            ValueResultOutcome outcome,
+            // TODO: Error should be an Enumerable<Error> and perhaps should not be here at all, only on ParseResult
+            string? error = null)
+        :this((CliSymbol)argument, value, locations, outcome,error)
+    {   }
+
+    /// <summary>
+    /// Creates a new ValueResult instance
+    /// </summary>
+    /// <param name="option">The CliOption the value is for.</param>
+    /// <param name="value">The entered value.</param>
+    /// <param name="locations">The locations list.</param>
+    /// <param name="outcome">True if parsing and converting the value was successful.</param>
+    /// <param name="error">The CliError if parsing or converting failed, otherwise null.</param>
+    internal ValueResult(
+        CliOption option,
+        object? value,
+        IEnumerable<Location> locations,
+        ValueResultOutcome outcome,
+        // TODO: Error should be an Enumerable<Error> and perhaps should not be here at all, only on ParseResult
+        string? error = null)
+    : this((CliSymbol)option, value, locations, outcome, error)
+    { }
+
+    /// <summary>
+    /// The CliSymbol the value is for. This is always a CliOption or CliArgument.
+    /// </summary>
     public CliSymbol ValueSymbol { get; }
+
     internal object? Value { get; }
 
+    /// <summary>
+    /// Returns the value, or the default for the type.
+    /// </summary>
+    /// <typeparam name="T">The type to return</typeparam>
+    /// <returns>The value, cast to the requested type.</returns>
     public T? GetValue<T>()
         => Value is null
             ? default
             : (T?)Value;
 
-    // This needs to be a collection because collection types have multiple tokens and they will not be simple offsets when response files are used
-    // TODO: Consider more efficient ways to do this in the case where there is a single location
+    /// <summary>
+    /// Gets the locations at which the tokens that made up the value appeared.
+    /// </summary>
+    /// <remarks>
+    /// This needs to be a collection because collection types have multiple tokens and they will not be simple offsets when response files are used.
+    /// </remarks>
     public IEnumerable<Location> Locations { get; }
 
+    /// <summary>
+    /// True when parsing and converting the value was successful
+    /// </summary>
     public ValueResultOutcome Outcome { get; }
 
+    /// <summary>
+    /// Parsing and conversion errors when parsing or converting failed.
+    /// </summary>
     public string? Error { get; }
 
+    /// <summary>
+    /// Returns test suitable for display. 
+    /// </summary>
+    /// <returns></returns>
+    /// <exception cref="NotImplementedException"></exception>
     public IEnumerable<string> TextForDisplay()
     {
         throw new NotImplementedException();