command-line-api API review issues

trafficstars

Based on the feedback from @bartonjs :

[x] #1892
[ ] #1893
[x] #1894
[x] #1895
[x] #1896
[x] #1897
[x] #1898
[x] #1899
[x] #1900
[x] #1901
[ ] #1902
[x] #1903
[x] #1904
[x] #1905
[x] #1906
[x] #1907
[ ] #1908
[ ] #1909
[ ] #1910
[x] #1911
[x] #1912
[x] #1913
[ ] #1914
[x] #1915
[x] #1916
[x] #1917
[x] #1918
[x] #1919
[x] #1920
[x] #1921
[x] #1922
[x] #1923
[x] #1924
[x] #1925
[x] #1926
[x] #1927
[x] #1928
[x] #1929
[ ] #1930
[ ] #1931
[x] #1932
[ ] #1933
[ ] #1934
[ ] #1935
[x] #1936
[x] #1937
[ ] #1938
[x] #1939
[ ] #1940
[ ] #1942
[ ] #1943
[x] #1944
[x] #1945

Based on other feedback:

[x] #1963
[x] #1965
[x] #1971

Nov 02 '22 16:11 jonsequitur

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

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

CompletionContext

public abstract class CompletionContext
    public CommandLine.ParseResult ParseResult { get; }
    public String WordToComplete { get; }

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

It's a public type with no public ctor. Does it need to be public? https://github.com/dotnet/command-line-api/issues/1929

TextCompletionContext

public class TextCompletionContext : CompletionContext
    public String CommandLineText { get; }
    public Int32 CursorPosition { get; }
    public TextCompletionContext AtCursorPosition(Int32 position)

Similarly to TokenCompletionContext it has no public ctor and seems like it could be made internal. https://github.com/dotnet/command-line-api/issues/1929

ICompletionSource

public interface ICompletionSource
    public Collections.Generic.IEnumerable<CompletionItem> GetCompletions(CompletionContext context)

Are there any types that implement ICompletionSource but don't derive from Symbol? https://github.com/dotnet/command-line-api/issues/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()

Could you provide an example of hidden symbol?
Does 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.

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

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

Nov 08 '22 16:11 adamsitnik

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.

Nov 08 '22 16:11 KalleOlaviNiemitalo

What is the difference between Keyword and Value kinds?

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

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

Nov 08 '22 16:11 KalleOlaviNiemitalo

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.