Move PSVersionInfo.PSVersionName to first place #3562

iSazonov · 2017-04-14T09:39:01Z

We want PSVersion on first place and the rest sorted alphabetically

>$PSVersionTable

Name                           Value
----                           -----
PSVersion                      6.0.0-alpha
BuildVersion                   3.0.0.0
CLRVersion
GitCommitId                    v6.0.0-alpha.18-12-gacec58c0491019557864189ebf03324e58874d63-dirty
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
PSEdition                      Core
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

daxian-dbw · 2017-04-17T18:30:47Z

Quoted from @rkeithhill's comment

Consider putting PSVersion and PSEdition in the first and second positions. After that, I don't care much about the order. I suppose alphabetical after the first two would be fine.

iSazonov · 2017-04-17T18:37:38Z

Ok, tomorrow.

daxian-dbw · 2017-04-20T23:47:17Z

src/System.Management.Automation/engine/PSVersionInfo.cs

+                keyList.Sort();
+                MoveItemOnFirstPlace(PSVersionInfo.PSEditionName, keyList);
+                MoveItemOnFirstPlace(PSVersionInfo.PSVersionName, keyList);
+                return keyList.ToArray();


This implementation is a little bit heavy, it involves shuffling keyList's elements for 3 times (sort() and 2 MoveItemOnFirstPlace) + 2 collection allocation (ArrayList and ToArray).
Maybe the following pseudo code would be better?

Array arr = new string[base.Keys.Count]; int index = 2; foreach (string key in base.Keys) { switch (key) case PSVersionInfo.PSVersionName: arr[0] = key; break; case PSVersionInfo.PSEditionName: arr[1] = key; break; default: arr[index++] = key; break; } Array.Sort(arr, 2, arr.Length - 2; /* the equalityComparer passed in the constructor */); return arr;

It allocate only one collection, and elements in arr only get moved around once, within the Sort method.

The pseudo code is still not perfect, because we will create an array object every time Keys is accessed. If we want to go further to avoid that, we can create our own type that implements ICollectionand is immutable from outside (no public members to mutate it, just like the type of object that Hashtable.Keys returns -- Hashtable+KeyCollection). Then we can keep one instance of it with PSVersionHashTable, and always reuse and return the same instance from Keys property.

Thanks for help with the code!
I added the optimized sorting and description.

If we do Keys read-only we should do PSVersionTable read-only too. And it is a breaking change - currently we can add in PSVersionTable own version strings. For example, third-party module can add its strings in PSVersionTable. I think it's not worth and we can live without the Keys cache. What are your thoughts?

PSVersionTable doesn't need to be read-only for the Keys to be read-only. Hashtable.Keys returns an object of System.Collections.Hashtable+KeyCollection, which expose no public members for you to mutate it.
This would be a pure optimization and is not required for this fix.

daxian-dbw · 2017-04-20T23:48:19Z

src/System.Management.Automation/engine/PSVersionInfo.cs

@@ -57,7 +59,7 @@ static PSVersionInfo()
            s_psVersionTable = new PSVersionHashTable(StringComparer.OrdinalIgnoreCase);

            s_psVersionTable[PSVersionInfo.PSVersionName] = s_psV6Version;
-            s_psVersionTable["PSEdition"] = PSEditionValue;
+            s_psVersionTable[PSVersionInfo.PSEditionName] = PSEditionValue;
            s_psVersionTable["BuildVersion"] = GetBuildVersion();
            s_psVersionTable["GitCommitId"] = GetCommitInfo();
            s_psVersionTable["PSCompatibleVersions"] = new Version[] { s_psV1Version, s_psV2Version, s_psV3Version, s_psV4Version, s_psV5Version, s_psV51Version, s_psV6Version };


Why not have const variables for other keys: "BuildVersion", "GitCommitId" and "PSCompatibleVersions"? 😄

I did not want to distract the unrelated changes. But I agree to bring it into line if you don't mind.

daxian-dbw · 2017-04-21T19:06:28Z

@iSazonov, the implementation will break in 2 cases:

there are non-string keys -- string array won't work and sort will fail
the keys 'PSEdition' or 'PSVersion' get removed from $psversiontable -- possible array indexing failure.

I pushed a fix to your branch, and need your help to review the code to see if there are other issues.

iSazonov · 2017-04-21T19:40:44Z

Why we want to allow non-string keys? It seems the keys in $psversiontable is NAMEs - other semantics is misguided I believe.

iSazonov · 2017-04-21T19:45:44Z

And maybe move our logic in custom StringComparer?

daxian-dbw · 2017-04-21T22:21:19Z

Why we want to allow non-string keys?

We don't want non-string keys, but user can add any key-value pairs to $psversiontable, and can also remove any pairs from it. It shouldn't fail when that happens.

iSazonov · 2017-04-24T14:58:23Z

If we are faced with the keys of arbitrary type we would use LanguagePrimitives.ConvertTo.

In last commit I also have tried to move sorting logic to custom Comparer. It looks more simple 😄 If you dont agree feel free to revert.

daxian-dbw · 2017-04-24T22:59:05Z

src/System.Management.Automation/engine/PSVersionInfo.cs

+            public override int Compare(object x, object y)
+            {
+                              string xString = (string)LanguagePrimitives.ConvertTo(x, typeof(string), null);

            
+                string yString = (string)LanguagePrimitives.ConvertTo(y, typeof(string), null);


What if x or y are objects but calling LanguagePrimitives.ConvertTo on them returns PSVersion or PSEdition as the converted string?
I still prefer to do sorting only if keys are all strings. If there is a non-string key, we just give up sorting and return base.Keys.
Sorry for being picky with rare cases. Since we are creating a public accessible data structure, we have to consider whatever that might happen.

This is very useful for me. 😄 The more we did not analyse this deeply before.

All previous options that we had, nothing broke really. The only thing we do here is to change a human output to the screen.
Output (Out-Default) always perform conversion to strings. Even if we have non-string key, it will be converted to a string. And it seems better to even see them sorted.

If a user adds a record with key that is converted to "PSVersion" we see two entries. We could block it but it will be breaking change.
Also we should use LanguagePrimitives.ConvertTo<string>(Value) (#2678 (comment)) to avoid "strange" sorting.

$a=[pscustomobject]@{v1=123} $a $a.ToString() # It is empty! $PSVersionTable[$a]="asdfgh" $PSVersionTable Name Value ---- ----- PSVersion 6.0.0-alpha PSEdition Core @{v1=123} asdfgh BuildVersion 3.0.0.0 CLRVersion GitCommitId v6.0.0-alpha.18-36-gc367cb58d87a84597077fbbe9a5ded5310179878 PSCompatibleVersions {1.0, 2.0, 3.0, 4.0...} PSRemotingProtocolVersion 2.3 SerializationVersion 1.1.0.1 WSManStackVersion 3.0 $b=$a | Add-Member -MemberType ScriptMethod -Name ToString -Value {"PSVersion"} -PassThru -Force $PSVersionTable[$b]="zxcvbn" $PSVersionTable Name Value ---- ----- PSVersion zxcvbn PSVersion 6.0.0-alpha PSEdition Core @{v1=123} asdfgh BuildVersion 3.0.0.0 CLRVersion GitCommitId v6.0.0-alpha.18-36-gc367cb58d87a84597077fbbe9a5ded5310179878 PSCompatibleVersions {1.0, 2.0, 3.0, 4.0...} PSRemotingProtocolVersion 2.3 SerializationVersion 1.1.0.1 WSManStackVersion 3.0

@daxian-dbw Could you please continue?

Sorry @iSazonov, I was busy with some other tasks. I will get back to this one ASAP.

Good argument. I agree to have the PSVersionTableComparer for the sorting.

daxian-dbw · 2017-05-02T18:48:54Z

src/System.Management.Automation/engine/PSVersionInfo.cs

+            }
+        }
+
+        private class PSVersionTableComparer : Comparer<object>


Maybe just implement the IComparer interface?

For PSVersionTable? We only compare keys not PSVersionTables.

No, for PSVersionTableComparer. ArrayList.Sort(IComparer) takes an IComparer as argument.

daxian-dbw · 2017-05-02T18:50:57Z

src/System.Management.Automation/engine/PSVersionInfo.cs

+        {
+            public override int Compare(object x, object y)
+            {
+                string xString = (string)LanguagePrimitives.ConvertTo(x, typeof(string), null);


We should use CultureInfo.CurrentCulture for the formatProvider parameter.

daxian-dbw · 2017-05-02T18:55:16Z

src/System.Management.Automation/engine/PSVersionInfo.cs

+            {
+                string xString = (string)LanguagePrimitives.ConvertTo(x, typeof(string), null);
+                string yString = (string)LanguagePrimitives.ConvertTo(y, typeof(string), null);
+                if (PSVersionInfo.PSVersionName == xString)


Instead of ==, we should use PSVersionInfo.PSVersionName.Equals(xString, StringComparison.OrdinalIgnoreCase)

daxian-dbw · 2017-05-02T19:02:39Z

src/System.Management.Automation/engine/PSVersionInfo.cs

+                }
+                else
+                {
+                    return xString.CompareTo(yString);


Maybe use String.Compare(string strA, string strB, System.StringComparison comparisonType) so that you can specify StringComparison.OrdinalIgnoreCase.

daxian-dbw · 2017-05-02T19:06:45Z

#3654 made changes to PSVersionInfo.cs, so I'm afraid you need to rebase and resolve the conflicts.

daxian-dbw · 2017-05-02T21:41:59Z

src/System.Management.Automation/engine/PSVersionInfo.cs

-                Array.Sort(arr, StringComparer.OrdinalIgnoreCase);
-                return arr;
+                ArrayList keyList = new ArrayList(base.Keys);
+                keyList.Sort(new PSVersionTableComparer());


PSVersionHashTable can have a static readonly field to hold an instance of PSVersionTableComparer, so as to avoid creating a new comparer every time.

iSazonov · 2017-05-03T05:57:36Z

@daxian-dbw Conflicts was resolved. Also I moved tests to PSVersionTable.Tests.ps1 file.

daxian-dbw · 2017-05-03T17:11:44Z

src/System.Management.Automation/engine/PSVersionInfo.cs

+            }
+        }
+
+        private static PSVersionTableComparer keysComparer = new PSVersionTableComparer();


Please make it readonly.
It's recommended to put field declarations before constructors, and for a private static field, it should be named with the prfix s_ -- s_keysComparer. See s_psVersionTable in this file for an example.

daxian-dbw · 2017-05-03T17:12:42Z

src/System.Management.Automation/engine/PSVersionInfo.cs

+        }
+
+        private static PSVersionTableComparer keysComparer = new PSVersionTableComparer();
+        private class PSVersionTableComparer : Comparer<object>


Like we discussed, PSVersionTableComparer can just implement IComparer due to ArrayList.Sort(IComparer comparer).

daxian-dbw · 2017-05-03T17:14:34Z

src/System.Management.Automation/engine/PSVersionInfo.cs

@@ -5,6 +5,7 @@
 using System.Diagnostics;
 using System.Reflection;
 using System.Collections;
+using System.Collections.Generic;


Once you make PSVersionTableComparer implements IComparer instead of deriving from Comparer<object>, this using directive can be removed.

iSazonov · 2017-05-04T04:37:30Z

@daxian-dbw Thanks for review and usefull comments!

daxian-dbw · 2017-05-04T04:37:51Z

@iSazonov Thanks for the great fix! 👍

msftclas added the cla-already-signed label Apr 14, 2017

daxian-dbw self-assigned this Apr 17, 2017

Move PSVersionInfo.PSVersionName to first place

6dd8c70

iSazonov force-pushed the psversiontable2 branch from 8c116eb to 6dd8c70 Compare April 18, 2017 14:22

Move PSEdition on second place

b559cc3

daxian-dbw reviewed Apr 20, 2017

View reviewed changes

iSazonov and others added 3 commits April 21, 2017 14:44

Bring const vars into line

681398e

Small sort optimization

7af43e7

Handle non-string keys and when 'PSVersion' or 'PSEdition' is removed

bf24d20

Move logic in custom Comparer

33f906a

daxian-dbw reviewed Apr 24, 2017

View reviewed changes

daxian-dbw reviewed May 2, 2017

View reviewed changes

iSazonov added 2 commits May 3, 2017 10:23

Merge branch 'master' into psversiontable2

5e08bc8

Fix after review

4d5ded7

iSazonov force-pushed the psversiontable2 branch from b499d85 to 4d5ded7 Compare May 3, 2017 05:36

daxian-dbw reviewed May 3, 2017

View reviewed changes

Move to IComparer

b09e9b5

daxian-dbw approved these changes May 4, 2017

View reviewed changes

daxian-dbw merged commit 42cb8ba into PowerShell:master May 4, 2017

iSazonov deleted the psversiontable2 branch May 11, 2017 07:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move PSVersionInfo.PSVersionName to first place #3562

Move PSVersionInfo.PSVersionName to first place #3562

Move PSVersionInfo.PSVersionName to first place #3562

Move PSVersionInfo.PSVersionName to first place #3562

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment