-
Notifications
You must be signed in to change notification settings - Fork 409
Description
The sorting logic done here when building the default help text from the list of completions is backward:
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:
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<int> Main ( string[] argv)
{
RootCommand root = new ( "example" );
Option<int> 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:
CompletionContext theCompletionContext = CompletionContext.Empty;
IOrderedEnumerable<CompletionItem> 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:
10
20
30
5
6
Here is the fixed sorting code, as it applies to building HelpText (an empty context gets passed in):
IOrderedEnumerable<CompletionItem> 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:
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):
IOrderedEnumerable<CompletionItem> properlyOrderedWithPartialEntry = sampleCompletions
.OrderByDescending ( item => item.Label.IndexOfCaseInsensitive ( "2" ) )
.ThenBy ( item => item.SortText, StringComparer.OrdinalIgnoreCase );
foreach ( CompletionItem c in properlyOrderedWithPartialEntry )
{
Console.WriteLine ( c.Label );
}
Output:
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.
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.