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

Command Explorer doesn't sort like Show-Command #1993

Open
corbob opened this issue May 29, 2019 · 6 comments
Open

Command Explorer doesn't sort like Show-Command #1993

corbob opened this issue May 29, 2019 · 6 comments
Labels
Area-Command Explorer Issue-Enhancement A feature request (enhancement).

Comments

@corbob
Copy link
Contributor

corbob commented May 29, 2019

The original intent behind Command Explorer was to mimic Show-Command. I noticed today that Show-Command sorts with the hyphens being of higher sort value than other letters. While the Sort-Object that we use in the Command Explorer seems to strip out the hyphens and sorts based on the result. So you'll see some Convert-Toxxx and then ConvertTo-yyy followed by Convert-Tozzz.

System Details

This happens with Windows PowerShell 5.1 on Windows 10 1709 and insiders build 18898 as well as PowerShell Core 6.2. As such, I'm going to forego the vscode version and PSES.

Issue Description

Example of 4 made up functions:

PS> function ConvertTo-Lkj {}
PS> function Convert-ToLkj {}
PS> function Convert-Lkj {}
PS> function ConvertLkj {}

When running Show-Command and then sorting for lkj:
Show-Command sorting for made up cmdlets

When running the equivalent Get-Command code Get-Command *lkj | select name | Sort name | ogv:
Get-Command equivalent of Show-Command sorting for made up cmdlets

I have also tested this in pwsh with the same behavior:

PS> function ConvertTo-Lkj {}
PS> function Convert-ToLkj {}
PS> function Convert-Lkj {}
PS> function ConvertLkj {}
PS> get-command *lkj | select name | sort name

Name
----
ConvertLkj
Convert-Lkj
ConvertTo-Lkj
Convert-ToLkj

Expected Behaviour

Output commands in the order:

Convert-Lkj
Convert-ToLkj
ConvertLkj
ConvertTo-Lkj

Actual Behaviour

Output commands in order:

ConvertLkj
Convert-Lkj
ConvertTo-Lkj
Convert-ToLkj
@rjmholt
Copy link
Collaborator

rjmholt commented May 29, 2019

@corbob What do you mean by hyphens included? As in Show-Command ignores hyphens?

This probably wouldn’t be considered a bug in Sort-Object but probably means we want to write our own sorting key function.

@corbob
Copy link
Contributor Author

corbob commented May 30, 2019

🤔 it looks like it's .Net's way of sorting is not what I was expecting. When I use CSI to do some impromptu C# sorting (and type stuff wrong and such) I see that it appears to sort in the same way:
image

Whereas the way I expected it to sort is along the lines of JavaScript:
image
Where the hyphens are treated as being higher in order than letters so all of the a-* are grouped together.

Perhaps this is an easy fix of sorting on the vscode side with TypeScript (which presumably sorts the same way as JavaScript ¯\(ツ)/¯ ).

Or alternatively we accept that it sorts differently and leave it as is.

@rjmholt
Copy link
Collaborator

rjmholt commented May 30, 2019

I'd suggest something like this:

internal class HyphenlessStringComparer : IComparer<string>
{
    public int Compare(string a, string b)
    {
        if (a == b) { return 0; }
        if (a == null) { return -1; }
        if (b == null) { return 1; }
        return a.Replace("-","").CompareTo(b.Replace("-",""));
    }
}

...

var strs = new List<string>(){ "ConvertLkj", "Convert-Lkj", "ConvertTo-Lkj", "Convert-ToLkj" };
var comparer = new HyphenlessStringComparer();
strs.Sort(comparer);

@rjmholt
Copy link
Collaborator

rjmholt commented May 30, 2019

(That can be improved if we want, so that new strings aren't allocated for the comparison. But it's more logic to maintain)

@rjmholt
Copy link
Collaborator

rjmholt commented May 30, 2019

Oh I see that's not quite the sort order you're looking for.

Can you describe formally what the sort order you want is?

Is it just a system where - has a lower ordinal than alphanumerics?

In any case, the fix is pretty straightforward, we just need to specify exactly what sort order you expect.

@corbob
Copy link
Contributor Author

corbob commented May 31, 2019

Formally the sort appears to be called Ordinal in the StringComparer class... It feels like it's probably a little more complicated for us because we have a List<PSCommandMessage> instead of List<String>.

void Main()
{
    List<String> L = new List<String> {"aa", "a-a", "a-b", "ab", "ba", "bb", "b-a"};
    L.Sort(StringComparer.Ordinal);
    foreach (var s in L)
    {
        System.Console.WriteLine(s);
    }
}

I think the easiest way to outline the difference I'm seeing is this screenshot:

image

Here you can see PowerShell on the left sorting the commands and you have Convert-Path, Convert-PSUObject, Convert-String in the middle, while Show-Command on the right puts them first. It feels as though PowerShell decided it was comparing ConvertFromStringData and ConvertPath instead of ConvertFrom-StringData and Convert-Path.

So the point I'm now at, after stewing over it and trying to wrap my head around how to describe what it's doing and what I think it should be doing is this: does it matter that much? As I stated originally, the intent was to mimic Show-Command as much as possible. However, now I wonder if we should instead be looking to move past Show-Command and make it our own. Perhaps we let the sort be unless there are other users that encounter it and think it's an issue (honestly, I'm probably the only person currently using it because I'm working on a WebView to insert the command with parameters).

@rjmholt rjmholt added Issue-Enhancement A feature request (enhancement). and removed Needs-Repro-Info labels Jun 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Command Explorer Issue-Enhancement A feature request (enhancement).
Projects
None yet
Development

No branches or pull requests

3 participants