dotnet / dotnet/command-line-api

CompletionSources not sorted properly when building HelpName default suggestions

Open
#2,705 2 comments 1 reaction 0 assignees View on GitHub
Dominant language
C#
Stars
3.7k
Forks
428
PR merge metrics
No merged PRs in 30d

Description

The sorting logic done here when building the default help text from the list of completions is backward:

https://github.com/dotnet/dotnet/blob/2db1f5ee2bdda2e8d873769325fabede32e420e0/src/command-line-api/src/System.CommandLine/Option.cs#L142

Since the ThenBy is on the Label, the resulting sorted collection is re-sorted by Label, as a primary order, not as a secondary order like SQL.

And, to hoist the current entry to the top of the list, the OrderBy needs to be OrderByDescending.

Here is an example that shows the bad behavior:

```cs

public static class Program
{
static CompletionItem[] sampleCompletions =
[
new ( "5", sortText: "05" ),
new ( "10", sortText: "10" ),
new ( "6", sortText: "06" ),
new ( "20", sortText: "10" ),
new ( "30", sortText: "30" )
];

public static async Task Main ( string[] argv)
{
RootCommand root = new ( "example" );
Option option = new ( "option" );
option.CompletionSources.Add ( static ctx => sampleCompletions );
root.Options.Add ( option );
await root.Parse ( [ "--help" ] ).InvokeAsync ( );
}
}

Output:
```text
Description:
example

Usage:
SnapsInAZfs [options]

Options:
option <10|20|30|5|6>
-?, -h, --help Show help and usage information
--version Show version information
```

Note the improperly ordered suggestions in the HelpName for `option`.

Here is why, isolated to what the line referenced at the top of the issue does:

```cs
CompletionContext theCompletionContext = CompletionContext.Empty;
IOrderedEnumerable improperlyOrdered = sampleCompletions
.OrderBy ( item => item.SortText.IndexOfCaseInsensitive ( theCompletionContext.WordToComplete ) )
.ThenBy ( item => item. Label, StringComparer.OrdinalIgnoreCase );

foreach ( CompletionItem c in improperlyOrdered )
{
Console.WriteLine ( c.Label );
}
```

Output:

```text
10
20
30
5
6
```

Here is the fixed sorting code, as it applies to building HelpText (an empty context gets passed in):

```cs
IOrderedEnumerable properlyOrdered = sampleCompletions
.OrderByDescending ( item => item.Label.IndexOfCaseInsensitive ( theCompletionContext.WordToComplete ) )
.ThenBy ( item => item.SortText, StringComparer.OrdinalIgnoreCase );

foreach ( CompletionItem c in properlyOrdered )
{
Console.WriteLine ( c.Label );
}
```

Output:

```text
5
6
10
20
30
```

And here is how it would look during tab completion, if the user had already entered "2" and then presses tab (simulated by giving it the "2" directly):

```cs
IOrderedEnumerable properlyOrderedWithPartialEntry = sampleCompletions
.OrderByDescending ( item => item.Label.IndexOfCaseInsensitive ( "2" ) )
.ThenBy ( item => item.SortText, StringComparer.OrdinalIgnoreCase );

foreach ( CompletionItem c in properlyOrderedWithPartialEntry )
{
Console.WriteLine ( c.Label );
}

```

Output:

```text
20
5
6
10
30
```

~~The actual code to replace what is there now would be:~~\
This is incorrect. See my follow-up comment that fixes it.

```cs
return completions
.OrderByDescending(item => item.Label.IndexOfCaseInsensitive(context.WordToComplete))
.ThenBy(symbol => symbol.SortText, StringComparer.OrdinalIgnoreCase);
```

Also, it may be worth pointing out that the extension method `IndexOfCaseInsensitive` does not use the same StringComparer as the code here. It uses invariant culture. This uses ordinal. So the two sorts aren't the same, necessarily, even if the values are identical.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.