Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API review issues #1891

Open
41 of 56 tasks
jonsequitur opened this issue Nov 2, 2022 · 15 comments
Open
41 of 56 tasks

API review issues #1891

jonsequitur opened this issue Nov 2, 2022 · 15 comments
Labels
Milestone

Comments

@jonsequitur
Copy link
Contributor

jonsequitur commented Nov 2, 2022

Based on the feedback from @bartonjs :

Based on other feedback:

@adamsitnik
Copy link
Member

I've noted some additional questions today:

Completions

CompletionItem

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()
  1. What is the difference between 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.
  2. Does providing SortText as a string makes it actually simpler to sort completions? Why not an int or enum? Is it actually used?
  3. The type overrides Equals, but does not implement IEqutable.
  4. The type is used for sorting, but does not implement IComparable.
  5. Documentation describes the item. Symbol defines Description. Should we unify them?
  6. In what scenarios Detail is needed? I would expect Documentation to be sufficient?
  7. How exactly InsertText works? Is it ever different than Label?

CompletionContext

public abstract class CompletionContext
    public CommandLine.ParseResult ParseResult { get; }
    public String WordToComplete { get; }
  1. 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?

TokenCompletionContext

public class TokenCompletionContext : CompletionContext
  1. It's a public type with no public ctor. Does it need to be public? Consider differentiating these two *CompletionContext scenarios using fewer public types #1929

TextCompletionContext

public class TextCompletionContext : CompletionContext
    public String CommandLineText { get; }
    public Int32 CursorPosition { get; }
    public TextCompletionContext AtCursorPosition(Int32 position)
  1. Similarly to TokenCompletionContext it has no public ctor and seems like it could be made internal. Consider differentiating these two *CompletionContext scenarios using fewer public types #1929

ICompletionSource

public interface ICompletionSource
    public Collections.Generic.IEnumerable<CompletionItem> GetCompletions(CompletionContext context)
  1. Are there any types that implement ICompletionSource but don't derive from Symbol? Consider whether ICompletionSource can be replaced with a delegate or class #1928

Edit: 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.

Symbols

Symbol

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()
  1. Could you provide an example of hidden symbol?
  2. Does Parent hierarchy is used by customers outside of S.CL? Can it be made internal?
  3. 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.

Argument

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()
  1. 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" } }
}
  1. 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?

  2. 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);
}

@KalleOlaviNiemitalo
Copy link

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:

  • If someone tries to use the option without looking at help, a custom validator can explain why it is not available.
  • Don't let Linux users fall into a habit of abbreviating some other option in such a way that the abbreviation becomes ambiguous on Windows where this option is available.

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Nov 8, 2022

What is the difference between Keyword and Value kinds?

Related #1765.

Does providing SortText as a string makes it actually simpler to sort completions?

Related #1220 (comment).

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.

@KalleOlaviNiemitalo
Copy link

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.

@KalleOlaviNiemitalo
Copy link

And why do we need Clear?

I need it in #1884.

@KalleOlaviNiemitalo
Copy link

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.

@KalleOlaviNiemitalo
Copy link

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.

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Nov 8, 2022

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 (#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.

@adamsitnik
Copy link
Member

@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 #1954 and let me know what do you think.

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Nov 8, 2022

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.

@adamsitnik
Copy link
Member

adamsitnik commented Nov 10, 2022

  1. Why do we expose a possibility to specify non-generic default value? It creates type mismatch risk?
Option<bool> runApiCompatOption = new("--run-api-compat", "....");
runApiCompatOption.SetDefaultValue(123); // providing int for a bool
  1. Do we really need 3 different ways of specyfing default value? (object value, action<object>, Func<ParseResult,object>)? How about just two?

  2. Argument is an arugment without name, but:

  • it defines Name (via Symbol)
  • it defines HelpName.

In what scenario the customer needs both Name and HelpName for Argument?

  1. Allowed values is defined as internal hashset of strings:
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)

Argument

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; }
  1. It has six public ctors, some of them define optional arguments, some do not. How about introducing just one with multiple optional arguments?

  2. 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?

ArgumentArity

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()
  1. Do you have any better name suggestions? It LGTM, but Jeremy pointed this out.

IdentifierSymbol

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)
  1. If we want to allow for hidden aliases, we should introduce the concept of Alias now. Any objections?

  2. 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);
  1. A lot of complexity comes from the fact that Name is mutable. Why? IMO we should make it readonly and initialized only by ctor.

Option

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)
  1. Argument defines HelpName, while Option: ArgumentHelpName. Why not just HelpName?

  2. Similarly to Argument, why non-generic Option allows for setting object defautl value or factory?

  3. Does HasAliasIgnoringPrefix really needs to be public? Why would any customer outside of S.CL use it?

Option

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; }
  1. Same as for Argumen<T>: why so many ctors? And the confusing bool isDefault?

  2. 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;
    }
}

Command

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()
  1. Add vs AddX. Which one do we keep?

  2. 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?

RootCommand

  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 = )
  1. All the properties it exposes are readonly, static and independent from user input. Do we really need RootCommand?

@KalleOlaviNiemitalo
Copy link

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.

if (valueDescriptor.ValueName.IsMatch(argument.Name))
public string ValueName => _parameterInfo.Name;
public string ValueName => _propertyInfo.Name;
var helpName = arg?.HelpName ?? string.Empty;
if (!string.IsNullOrEmpty(helpName))
{
firstColumn = helpName!;
}
else if (completions.Length > 0)
{
firstColumn = string.Join("|", completions);
}
else
{
firstColumn = argument.Name;
}

@tmds
Copy link
Member

tmds commented Mar 22, 2023

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.

@adamsitnik
Copy link
Member

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);
}

@tmds
Copy link
Member

tmds commented Mar 22, 2023

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants