Open jonsequitur opened 1 year ago
I've noted some additional questions today:
public class CompletionItem
.ctor(String label, String kind = Value, String sortText = null, String insertText = null, String documentation = null, String detail = null)
public String Detail { get; }
public String Documentation { get; set; }
public String InsertText { get; }
public String Kind { get; } // string, not enum, 2 values, used only for equality checks
public String Label { get; } // value, displayed to the users
public String SortText { get; } // when present, used for sorting the completions instead Label.
protected Boolean Equals(CompletionItem other)
public Boolean Equals(Object obj)
public Int32 GetHashCode()
public String ToString()
Keyword
and Value
kinds? It seems that Kind
could be made iternal as Keyword
is used only in once place and Kind
is used only by Equals
and GetHashCode
.SortText
as a string makes it actually simpler to sort completions? Why not an int or enum? Is it actually used?Equals
, but does not implement IEqutable
.IComparable
.Documentation
describes the item. Symbol defines Description
. Should we unify them?Detail
is needed? I would expect Documentation
to be sufficient?InsertText
works? Is it ever different than Label
?public abstract class CompletionContext
public CommandLine.ParseResult ParseResult { get; }
public String WordToComplete { get; }
WordToComplete
is very self-describing name, on the other it's part of CompletionContext
so I wonder whether we could simplify it: CompletionContext.Word
?public class TokenCompletionContext : CompletionContext
ctor
. Does it need to be public? https://github.com/dotnet/command-line-api/issues/1929public class TextCompletionContext : CompletionContext
public String CommandLineText { get; }
public Int32 CursorPosition { get; }
public TextCompletionContext AtCursorPosition(Int32 position)
TokenCompletionContext
it has no public ctor
and seems like it could be made internal
. https://github.com/dotnet/command-line-api/issues/1929public interface ICompletionSource
public Collections.Generic.IEnumerable<CompletionItem> GetCompletions(CompletionContext context)
ICompletionSource
but don't derive from Symbol
? https://github.com/dotnet/command-line-api/issues/1928Edit: I can see there are AnonymousCompletionSource
for representing plain strings and CompletionSourceForType
which more or less delegates to AnonymousCompletionSource
. So in theory we could remove ICompletionSource
nad introduce an artifical symbol for representing a simple completions source.
public abstract class Symbol, CommandLine.Completions.ICompletionSource
public String Description { get; set; }
public Boolean IsHidden { get; set; }
public String Name { get; set; }
public Collections.Generic.IEnumerable<Symbol> Parents { get; }
public Collections.Generic.IEnumerable<CommandLine.Completions.CompletionItem> GetCompletions()
public Collections.Generic.IEnumerable<CommandLine.Completions.CompletionItem> GetCompletions(CommandLine.Completions.CompletionContext context)
public String ToString()
Parent
hierarchy is used by customers outside of S.CL
? Can it be made internal
?GetCompletions(void)
is part of Symbol
, but GetCompletions(CompletionContext)
is part of ICompletionSource
iterface. Do we need both? We could make context
an optional argument or just remove GetCompletions(void)
as it seems to be used only for internal purposes.public abstract class Argument : Symbol, CommandLine.Binding.IValueDescriptor, CommandLine.Completions.ICompletionSource
public ArgumentArity Arity { get; set; }
public Collections.Generic.ICollection<CommandLine.Completions.ICompletionSource> Completions { get; }
public Boolean HasDefaultValue { get; }
public String HelpName { get; set; }
public Type ValueType { get; }
public Void AddValidator(Action<CommandLine.Parsing.ArgumentResult> validate)
public Collections.Generic.IEnumerable<CommandLine.Completions.CompletionItem> GetCompletions(CommandLine.Completions.CompletionContext context)
public Object GetDefaultValue()
public Void SetDefaultValue(Object value)
public Void SetDefaultValueFactory(Func<Object> defaultValueFactory)
public Void SetDefaultValueFactory(Func<CommandLine.Parsing.ArgumentResult,Object> defaultValueFactory)
public String ToString()
Completions
is a collection of ICompletionSource
, while all the usages in test project more or less provide a single source with multiple completion items. Should it be a collection of CompletionItem
instead?new Argument<string>
{
Completions = { _ => new[] { "vegetable", "mineral", "animal" } }
}
In most of the Completions
usages in test project the property is initialized only once, when the model is being created. Would it make sense to expose the possibility to add new completions only at creation time via ctor
? And why do we need Clear
? This could allow us for making Completions
private and an implementation detail and get closer to having symbols immutable?
The completions are sorted and verified for duplication every time GetCompletions
is called. How frequently is this method being called? Should we do it only once?
public override IEnumerable<CompletionItem> GetCompletions(CompletionContext context)
{
return Completions
.SelectMany(source => source.GetCompletions(context))
.Distinct()
.OrderBy(c => c.SortText, StringComparer.OrdinalIgnoreCase);
}
Could you provide an example of hidden symbol?
For a hidden Option\<bool>: I had a command-line application with an --out-clipboard
option that used System.Windows.Forms.Clipboard. Then I was porting it to net6.0 (not net6.0-windows) where Windows Forms is not available and the option cannot work. I set IsHidden = true
to hide the unusable option from help and completions. I could have put #if
around the whole option, but I chose to keep it in the parser for a few reasons:
What is the difference between Keyword and Value kinds?
Related https://github.com/dotnet/command-line-api/issues/1765.
Does providing SortText as a string makes it actually simpler to sort completions?
Related https://github.com/dotnet/command-line-api/issues/1220#issuecomment-1172911121.
How exactly InsertText works? Is it ever different than Label?
IIUC, a completion menu (perhaps a popup in PowerShell) could display Label and then insert InsertText. But the suggest directive currently uses Label only, so dotnet-suggest will complete incorrectly if the completion delegate sets different strings in them.
Help likewise uses Label only; I think that too should be InsertText. Even though the help is displayed to the user and Label therefore seems correct, it is intended for the user to copy to a command so InsertText is better. https://github.com/dotnet/command-line-api/blob/8b66431d25ec0667a052fa8635ef27adcefead9e/src/System.CommandLine/Help/HelpBuilder.Default.cs#L70
Nit on the one hand WordToComplete is very self-describing name, on the other it's part of CompletionContext so I wonder whether we could simplify it: CompletionContext.Word?
I feel this would suggest it is already a complete word, rather than part of a word.
And why do we need Clear?
I need it in https://github.com/dotnet/command-line-api/issues/1884.
The completions are sorted and verified for duplication every time GetCompletions is called. How frequently is this method being called? Should we do it only once?
AFAIK, GetCompletions is not normally called more than once for the same Symbol in the same process. It might be called again in an interactive application that reads more commands, but then it would likely have a new CompletionContext so you wouldn't be able to cache anyway.
GetCompletions(void) is part of Symbol, but GetCompletions(CompletionContext) is part of ICompletionSource iterface. Do we need both? We could make context an optional argument or just remove GetCompletions(void) as it seems to be used only for internal purposes.
I use Symbol.GetCompletions() for testing a custom completion callback; please do not remove this feature. The alternative Symbol.GetCompletions(CompletionContext) is not suitable because AFAICT there is no way for external code to create a ComplationContext instance; it and the two derived classes all have internal constructors.
Completions is a collection of ICompletionSource, while all the usages in test project more or less provide a single source with multiple completion items. Should it be a collection of CompletionItem instead?
No, the completion items can depend on the parse results of other symbols. Like an Argument\<FileInfo> specifies a file and an Option\<string> specifies an identifier defined in that file, and the completion delegate reads the file to find out what identifiers are defined there and what completion items should be generated. Also, I believe System.CommandLine currently requires the app to set up the arguments and options of all subcommands in advance (https://github.com/dotnet/command-line-api/issues/1956), and if you require it to generate all the completion items in advance too, that will cost time during startup.
I think a property public Func<CompletionContext, IEnumerable<CompletionItem>> Completer { get; set; }
would be OK, though. Multiple completion sources don't seem very common, and an app developer who wants multiple can set up a wrapper that calls them in the desired order.
@KalleOlaviNiemitalo big thanks for all the answers!
I use Symbol.GetCompletions() for testing a custom completion callback; please do not remove this feature. The alternative Symbol.GetCompletions(CompletionContext) is not suitable because AFAICT there is no way for external code to create a ComplationContext instance; it and the two derived classes all have internal constructors.
PTAL at https://github.com/dotnet/command-line-api/pull/1954 and let me know what do you think.
Why CompletionItem.Label would ever differ from InsertText:
Localization. InsertText could be culture-invariant so that a shell script works in all cultures, while Label would be localised.
Quoting. Maybe Label is @op
but InsertText is @@op
to quote it for the response file feature. OTOH, I don't think this kind of quoting should be done by the completion delegates; they should be agnostic of parser configuration. Likewise, they should not quote $var
as \$var
because it's not their business to know what kind of shell the user is using.
Option<bool> runApiCompatOption = new("--run-api-compat", "....");
runApiCompatOption.SetDefaultValue(123); // providing int for a bool
Do we really need 3 different ways of specyfing default value? (object value
, action<object>
, Func<ParseResult,object>
)? How about just two?
Argument is an arugment without name, but:
In what scenario the customer needs both Name and HelpName for Argument?
internal HashSet<string>? AllowedValues { get; private set; }
and it's used by two extension methods: FromAmong
. Could it be removed and implemented using a validator? (it's impl detail as it's currently not exposed publicly)
public class Argument<T> : Argument, IValueDescriptor<T>, System.CommandLine.Binding.IValueDescriptor
.ctor()
.ctor(System.String name, System.String description = null)
.ctor(System.String name, Func<T> defaultValueFactory, System.String description = null)
.ctor(Func<T> defaultValueFactory)
.ctor(System.String name, Func<System.CommandLine.Parsing.ArgumentResult,T> parse, System.Boolean isDefault = False, System.String description = null)
.ctor(Func<System.CommandLine.Parsing.ArgumentResult,T> parse, System.Boolean isDefault = False)
public System.Type ValueType { get; }
It has six public ctors, some of them define optional arguments, some do not. How about introducing just one with multiple optional arguments?
I find it confusing to use a boolean flag to tell the argument to use its custom parser as default value factory.
/// <param name="parse">A custom argument parser.</param>
/// <param name="isDefault"><see langword="true"/> to use the <paramref name="parse"/> result as default value.</param>
I would expect that any custom parser can just return default value if they want to?
public struct ArgumentArity : System.ValueType, System.IEquatable<ArgumentArity>
public static ArgumentArity ExactlyOne { get; }
public static ArgumentArity OneOrMore { get; }
public static ArgumentArity Zero { get; }
public static ArgumentArity ZeroOrMore { get; }
public static ArgumentArity ZeroOrOne { get; }
.ctor(System.Int32 minimumNumberOfValues, System.Int32 maximumNumberOfValues)
public System.Int32 MaximumNumberOfValues { get; }
public System.Int32 MinimumNumberOfValues { get; }
public System.Boolean Equals(ArgumentArity other)
public System.Boolean Equals(System.Object obj)
public System.Int32 GetHashCode()
public abstract class IdentifierSymbol : Symbol
public System.Collections.Generic.IReadOnlyCollection<System.String> Aliases { get; }
public System.String Name { get; set; }
public System.Void AddAlias(System.String alias)
public System.Boolean HasAlias(System.String alias)
If we want to allow for hidden aliases, we should introduce the concept of Alias
now. Any objections?
Perf: it always allocates a HashSet. We need to find a way to avoid doing that (I have tried once and failed but it's doable).
private protected readonly HashSet<string> _aliases = new(StringComparer.Ordinal);
Name
is mutable. Why? IMO we should make it readonly and initialized only by ctor
.public abstract class Option : IdentifierSymbol, System.CommandLine.Binding.IValueDescriptor
public System.Boolean AllowMultipleArgumentsPerToken { get; set; }
public System.String ArgumentHelpName { get; set; }
public ArgumentArity Arity { get; set; }
public System.Boolean IsRequired { get; set; }
public System.Type ValueType { get; }
public System.Void AddValidator(System.Action<System.CommandLine.Parsing.OptionResult> validate)
public System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem> GetCompletions(System.CommandLine.Completions.CompletionContext context)
public System.Boolean HasAliasIgnoringPrefix(System.String alias)
public System.Void SetDefaultValue(System.Object value)
public System.Void SetDefaultValueFactory(System.Func<System.Object> defaultValueFactory)
Argument defines HelpName
, while Option
: ArgumentHelpName
. Why not just HelpName
?
Similarly to Argument
, why non-generic Option
allows for setting object
defautl value or factory?
Does HasAliasIgnoringPrefix
really needs to be public? Why would any customer outside of S.CL use it?
public class Option<T> : Option, IValueDescriptor<T>, System.CommandLine.Binding.IValueDescriptor
.ctor(System.String name, System.String description = null)
.ctor(System.String[] aliases, System.String description = null)
.ctor(System.String name, Func<System.CommandLine.Parsing.ArgumentResult,T> parseArgument, System.Boolean isDefault = False, System.String description = null)
.ctor(System.String[] aliases, Func<System.CommandLine.Parsing.ArgumentResult,T> parseArgument, System.Boolean isDefault = False, System.String description = null)
.ctor(System.String name, Func<T> defaultValueFactory, System.String description = null)
.ctor(System.String[] aliases, Func<T> defaultValueFactory, System.String description = null)
public ArgumentArity Arity { get; set; }
Same as for Argumen<T>
: why so many ctors? And the confusing bool isDefault
?
Arity seems to be redundant as it does exactly the same thing as the base type implementation?
public class Option
{
/// <summary>
/// Gets or sets the arity of the option.
/// </summary>
public virtual ArgumentArity Arity
{
get => Argument.Arity;
set => Argument.Arity = value;
}
}
public class Option<T>
{
/// <inheritdoc/>
public override ArgumentArity Arity
{
get => base.Arity;
set => Argument.Arity = value;
}
}
public class Command : IdentifierSymbol, System.Collections.Generic.IEnumerable<Symbol>, System.Collections.IEnumerable
.ctor(System.String name, System.String description = null)
public System.Collections.Generic.IReadOnlyList<Argument> Arguments { get; }
public System.Collections.Generic.IEnumerable<Symbol> Children { get; }
public ICommandHandler Handler { get; set; }
public System.Collections.Generic.IReadOnlyList<Option> Options { get; }
public System.Collections.Generic.IReadOnlyList<Command> Subcommands { get; }
public System.Boolean TreatUnmatchedTokensAsErrors { get; set; }
public System.Void Add(Option option)
public System.Void Add(Argument argument)
public System.Void Add(Command command)
public System.Void AddArgument(Argument argument)
public System.Void AddCommand(Command command)
public System.Void AddGlobalOption(Option option)
public System.Void AddOption(Option option)
public System.Void AddValidator(System.Action<System.CommandLine.Parsing.CommandResult> validate)
public System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem> GetCompletions(System.CommandLine.Completions.CompletionContext context)
public System.Collections.Generic.IEnumerator<Symbol> GetEnumerator()
Add
vs AddX
. Which one do we keep?
Why does it need to implement IEnumerable<Symbol>
and expose IEnumerable<Symbol> Children
at the same time? Is it because the duck typing for C# syntax?
public class RootCommand : Command, System.Collections.Generic.IEnumerable<Symbol>, System.Collections.IEnumerable
public static System.String ExecutableName { get; }
public static System.String ExecutablePath { get; }
.ctor(System.String description = )
RootCommand
?In what scenario the customer needs both Name and HelpName for Argument?
Argument.Name is culture-invariant and can be compared to names from reflection, while Argument.HelpName is localized and displayed in help.
https://github.com/dotnet/command-line-api/blob/e2cdee38691892794dd4ec64a4364ffe21838a48/src/System.CommandLine.NamingConventionBinder/CommandResultExtensions.cs#L21 https://github.com/dotnet/command-line-api/blob/e2cdee38691892794dd4ec64a4364ffe21838a48/src/System.CommandLine.NamingConventionBinder/ParameterDescriptor.cs#L26 https://github.com/dotnet/command-line-api/blob/e2cdee38691892794dd4ec64a4364ffe21838a48/src/System.CommandLine.NamingConventionBinder/PropertyDescriptor.cs#L23 https://github.com/dotnet/command-line-api/blob/e2cdee38691892794dd4ec64a4364ffe21838a48/src/System.CommandLine/Help/HelpBuilder.Default.cs#L74-L87
All the properties it exposes are readonly, static and independent from user input. Do we really need RootCommand?
Besides the properties, another difference is Command
expects the commands to be named, and RootCommand
does not require a name on construction. I assume that is the reason the type exists.
Independent of whether the type stays or goes, it would be nice that APIs accept a Command
instead of a RootCommand
. I have a derived Command
type, and currently I can't pass it to APIs that explicitly require a RootCommand
.
Any additional properties (if ever needed) can be added to CommandLineConfiguration
instead of RootCommand
.
Independent of whether the type stays or goes, it would be nice that APIs accept a Command instead of a RootCommand
That is definitely one of our goals.
Besides the properties, another difference is Command expects the commands to be named, and RootCommand does not require a name on construction.
I was just thinking about creating a parameterless Command
ctor, but it would make it easy to use it by accident. An alternative is creating a static factory method on the Command
:
class Command
{
public static Command CreateRootCommand() => new Command($appName);
}
An alternative is creating a static factory method on the Command:
Yes, and if you prefer to not have factory methods. You could keep the RootCommand
for the sole purpose of being that factory. (That is: make it sealed
and only have a constructor.)
Based on the feedback from @bartonjs :
Based on other feedback: