8000 Invariable order of $PSVersionTable by iSazonov · Pull Request #3530 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]

Skip to content

Invariable order of $PSVersionTable #3530

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

Merged
merged 2 commits into from
Apr 13, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
8000 Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 40 additions & 3 deletions src/System.Management.Automation/engine/PSVersionInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ internal class PSVersionInfo
internal const string PSVersionName = "PSVersion";
internal const string SerializationVersionName = "SerializationVersion";
internal const string WSManStackVersionName = "WSManStackVersion";
private static Hashtable s_psVersionTable = null;
private static PSVersionHashTable s_psVersionTable = null;

/// <summary>
/// A constant to track current PowerShell Version.
Expand Down Expand Up @@ -54,7 +54,7 @@ internal class PSVersionInfo
// Static Constructor.
static PSVersionInfo()
{
s_psVersionTable = new Hashtable(StringComparer.OrdinalIgnoreCase);
s_psVersionTable = new PSVersionHashTable(StringComparer.OrdinalIgnoreCase);

s_psVersionTable[PSVersionInfo.PSVersionName] = s_psV6Version;
s_psVersionTable["PSEdition"] = PSEditionValue;
Expand All @@ -71,7 +71,7 @@ static PSVersionInfo()
#endif
}

internal static Hashtable GetPSVersionTable()
internal static PSVersionHashTable GetPSVersionTable()
{
return s_psVersionTable;
}
Expand Down Expand Up @@ -315,6 +315,43 @@ internal static SemanticVersion PSV6Version
#endregion
}

/// <summary>
/// Represents an implementation of '$PSVersionTable' variable.
/// The implementation contains ordered 'Keys' and 'GetEnumerator' to get user-friendly output.
/// </summary>
public sealed class PSVersionHashTable : Hashtable, IEnumerable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public class Hashtable : IDictionary, ICollection, IEnumerable, ...

Hashtable implements IEnumerable, so do you still need to specify IEnumerable here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VS Code detects this as error and build failed too without explicity adding IEnumerable.

{
internal PSVersionHashTable(IEqualityComparer equalityComparer) : base(equalityComparer)
{
}

/// <summary>
/// Returns ordered collection with Keys of 'PSVersionHashTable'
/// </summary>
public override ICollection Keys
{
get
{
Array arr = new string[base.Keys.Count];
base.Keys.CopyTo(arr, 0);
Array.Sort(arr, StringComparer.OrdinalIgnoreCase);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I like this sorting - today PSVersion shows up first, or at least it should.
With this change, it will be somewhat harder to see

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree 100%. About the only thing I ever look for from this info is PSVersion. This will make it harder to see that version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe an OrderedDictionary should be used here.

Copy link
Collaborator Author
@iSazonov iSazonov Apr 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have several open discussions on adding additional rows. Alphabetical order will be even more useful after that.

If you want only PSVersion why don't use $PSVersionTable.PSVersion?

I wonder how it makes Windows PowerShell for PSVersionHashTable hashtable ?

I could slightly change this code to move PSVersion in the first place without moving to OrderedDictionary

Update: Moving to OrderedDictionary may be not easy. I review the code - GetPSVersionTableForDownLevel is used in serialization.cs and it is based on hashtable. So I didn't even imagine that there can be broken.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: Moving to OrderedDictionary may be not easy. I review the code - GetPSVersionTableForDownLevel is used in serialization.cs and it is based on hashtable. So I didn't even imagine that there can be broken.

This is a good point. Thanks @iSazonov

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return arr;
}
}

/// <summary>
/// Returns an enumerator for 'PSVersionHashTable'.
/// The enumeration is ordered (based on ordered version of 'Keys').
/// </summary>
IEnumerator IEnumerable.GetEnumerator()
{
foreach (string key in Keys)
{
yield return new System.Collections.DictionaryEntry(key, this[key]);
}
}
}

/// <summary>
/// An implementation of semantic versioning (http://semver.org)
/// that can be converted to/from <see cref="System.Version"/>.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,14 @@ Describe "Validate special variables" -Tags "CI" {
It "Verify `$PSVersionTable.PSEdition" {
$PSVersionTable["PSEdition"] | Should Be "Core"
}

It "Verify `$PSVersionTable is ordered" {
$keys1 = $PSVersionTable.Keys
$keys1sorted = $keys1 | Sort-Object
$keys2 = ($PSVersionTable | Format-Table -HideTableHeaders -Property Name | Out-String) -split [System.Environment]::NewLine | Where-Object {$_}
$keys2sorted = $keys2 | Sort-Object

Compare-Object -ReferenceObject $keys1 -DifferenceObject $keys1sorted -SyncWindow 0 | Should Be $null
Compare-Object -ReferenceObject $keys2 -DifferenceObject $keys2sorted -SyncWindow 0 | Should Be $null
}
}
0