Make SemanticVersion compatible with SemVer 2.0 by iSazonov · Pull Request #5037 · PowerShell/PowerShell

iSazonov · 2017-10-06T15:36:35Z

Fix #5004

Fix description

Made compatible with SymVer 2.0 http://semver.org/ (p.10)
Fixed comparisons
Refactored tests
Added new tests

Additional information

Useful sample https://github.com/maxhauser/semver/blob/master/src/Semver/SemVersion.cs
Related #2983

daxian-dbw · 2017-11-01T04:26:14Z

@JamesWTruher @PaulHigin Can you please review this PR?

PaulHigin

I am not quite finished with the review.

PaulHigin · 2017-11-01T20:40:59Z

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

Typo: The patch version

PaulHigin · 2017-11-01T21:03:26Z

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

We don't need to explicitly set properties to null since CLR automatically initializes fields to zero.

I only thought about symmetry and better readability.

Removed.

PaulHigin · 2017-11-01T21:06:17Z

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

Shouldn't these static strings also be const string? They are never reassigned so there is no point in allocating.

PaulHigin · 2017-11-01T21:31:26Z

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

Can you include examples of allowed label strings in the comment? I don't read regex very well :)

Complex RegEx is a puzzle for me too. I found some sample RegEx strings for SymVer but after testing I concluded that they all have bugs.
So I split the SymVer regex string on some string (s_VersionSansRegEx, s_LabelRegEx, s_LabelUnitRegEx). This slightly increased the code but helped to avoid bugs.

Comment added.

PaulHigin · 2017-11-01T21:33:07Z

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

The Group.Value always returns an empty string for no matches. Should the PreLable/BuildLable properties be initialized to empty string rather than null?

I see that we always check strings for NullOrEmpty so I think we are Ok as is.

PaulHigin · 2017-11-01T22:33:37Z

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

I wonder if we should do this preLabel, buildLabel regex validation in the SemanticVersion constructor. That way we can prevent malformed SemanticVersion objects from being created.

I wonder too why I checked only first char in constructors :-) I wonted to be fast. We should use RegEx.

Fixed.

PaulHigin · 2017-11-01T22:47:04Z

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

I feel we still need this check, otherwise v1.CompareTo will throw null reference exception.

Fixed by new method Compare()

PaulHigin · 2017-11-01T22:47:39Z

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

I feel we still need this check, otherwise v1.CompareTo will throw null reference exception.

PaulHigin · 2017-11-01T22:47:55Z

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

We should check v1 for null value.

PaulHigin · 2017-11-01T22:48:07Z

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

We should check v1 for null value.

PaulHigin · 2017-11-02T15:56:38Z

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

Please add a comment that PreLabel takes lower precedence when normal versions are equal, per spec. This logic always confuses me until I remember that :)

Actually this PreLabel null check here can be removed since it is duplicated in ComparePreLabel().

But please add the comment in ComparePreLabel() method.

Duplicate checks was removed.
Comment was added.

Replace static strings with const. Add strong checks in constructors. Add comments. Add static Compare() and fix comparisions operators.

iSazonov · 2017-11-05T11:21:09Z

I've rebased to resolve a merge conflict.

daxian-dbw · 2017-11-06T16:34:31Z

@anmenaga Can you please also review this PR? Thanks!

daxian-dbw · 2017-11-06T18:50:04Z

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

+        /// <param name="preLabel">The preLabel for the version</param>
+        /// <param name="buildLabel">The buildLabel for the version</param>
+        /// <exception cref="FormatException">
+        /// If <paramref name="preLabel"/> starts with '-' ot contains '+'.


'-' ot contains

Typo here.

I changed the comments because the code was changed previously.

daxian-dbw · 2017-11-06T18:53:57Z

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

    {
+        private const string s_VersionSansRegEx = @"^(?<major>\d+)(\.(?<minor>\d+))?(\.(?<patch>\d+))?$";
+        private const string s_LabelRegEx = @"^((?<preLabel>[0-9A-Za-z][0-9A-Za-z\-\.]*))?(\+(?<buildLabel>[0-9A-Za-z][0-9A-Za-z\-\.]*))?$";
+        private const string s_LabelUnitRegEx = @"^[0-9A-Za-z][0-9A-Za-z\-\.]*?$";


The variable names should be changed. s_ indicates static private variables, but they are const variables.

-.]*?$

Since you are matching to the end of line, is the question mark ? (lazy matching) needed?

s_ was removed.
? was removed - I think it is bug in any case.

daxian-dbw · 2017-11-06T21:18:25Z

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

+        /// PreLabel position in the SymVer string 'major.minor.patch-PreLabel+BuildLabel'.
+        /// </summary>
+
+        public string PreLabel { get; }


Maybe we should just the full name PrereleaseLabel for the public property?

Replaced with PreReleaseLabel - it looks more readable.

daxian-dbw · 2017-11-06T21:27:44Z

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

-        /// The last component in a SemanticVersion - may be null if not specified.
+        /// PreLabel position in the SymVer string 'major.minor.patch-PreLabel+BuildLabel'.
+        /// </summary>
+


Can you please remove the extra new line?

daxian-dbw · 2017-11-06T22:04:48Z

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

+        /// </summary>
+        public static int Compare(SemanticVersion versionA, SemanticVersion versionB)
+        {
+            if (versionA == null) return -1;


what about versionA and versionB are both null?

Good catch!

Fixed.

daxian-dbw · 2017-11-06T22:06:26Z

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

-                Minor.GetHashCode(),
-                Patch.GetHashCode(),
-                Label == null ? 0 : Label.GetHashCode());
+            return versionString.GetHashCode();


versionString could be null when running this method.

Besides, if we use the hashcode of versionString for the SemanticVersion, we would have a hashcode collision, would that be a problem? Why not continue to use Utils.CombineHashCodes?

null - fixed.

versionString is relatively short string - why you believe that CoreFX implementation of String.GetHashCode() so bad that we should make more complex hashcode?

What I mean is the hashcode of a SemanticsVersion instance would be the same of the hashcode of a string (the versionString), so there is a collision in terms that 2 different objects have the same hashcode.

In the docs I don't found that the hash must be different for different objects of different types. It seems we follow the docs - it will be work well with Dictionary and Hashtable types.

Maybe I'm just overthinking it. Let's keep it as is. #Close

daxian-dbw · 2017-11-06T22:17:55Z

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

+                }
+                else
+                {
+                    if (isNumber1) { return -1; }


It would be great if you can add comment here saying that "Numeric identifiers always have lower precedence than non-numeric identifiers. "

I copy-paste all text from the standard.

daxian-dbw · 2017-11-07T17:19:23Z

src/System.Management.Automation/FormatAndOutput/DefaultFormatters/DotNetTypes_format_ps1xml.cs

+                        .AddPropertyColumn("Major")
+                        .AddPropertyColumn("Minor")
+                        .AddPropertyColumn("Patch")
+                        .AddPropertyColumn("PreLabel")


The property name has changed to 'PrereleaseLabel'.

daxian-dbw · 2017-11-07T17:20:11Z

src/System.Management.Automation/FormatAndOutput/DefaultFormatters/DotNetTypes_format_ps1xml.cs

+                        .AddPropertyColumn("Minor")
+                        .AddPropertyColumn("Build")
+                        .AddPropertyColumn("Revision")
+                        .AddPropertyColumn("PSSemanticVersionPreLabel")


Would it make sense to change this property name to PSSemanticVersionPrereleaseLabel?

Looks very long 😕

Maybe PSSemVerPreReleaseLabel?

That looks good to me, especially semver is the type accelerator for SemanticVersion.

daxian-dbw · 2017-11-07T17:24:28Z

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

+        /// If <paramref name="preLabel"/> don't match 'LabelUnitRegEx'.
+        /// If <paramref name="buildLabel"/> don't match 'LabelUnitRegEx'.
+        /// </exception>
+        public SemanticVersion(int major, int minor, int patch, string preLabel, string buildLabel)


Shall we rename preLabel to prereleaseLabel too? It's the parameter of a public method.

daxian-dbw · 2017-11-07T17:25:45Z

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

+        private const string VersionSansRegEx = @"^(?<major>\d+)(\.(?<minor>\d+))?(\.(?<patch>\d+))?$";
+        private const string LabelRegEx = @"^((?<preLabel>[0-9A-Za-z][0-9A-Za-z\-\.]*))?(\+(?<buildLabel>[0-9A-Za-z][0-9A-Za-z\-\.]*))?$";
+        private const string LabelUnitRegEx = @"^[0-9A-Za-z][0-9A-Za-z\-\.]*$";
+        private const string PreLabelPropertyName = "PSSemanticVersionPreLabel";


Same here. We probably should use PSSemanticVersionPrereleaseLabel, since it's for a public property.

daxian-dbw · 2017-11-07T17:30:18Z

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

+        /// </summary>
+        public static int Compare(SemanticVersion versionA, SemanticVersion versionB)
+        {
+            if (versionA != null) {


Minor comment: it's better to follow the same open curly brace style in this file.

daxian-dbw · 2017-11-07T17:37:11Z

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

+                if (versionB != null)
+                {
+                    return versionA.CompareTo(versionB);
+                }


The instance method CompareTo handles null value in it, so this versionB != null check is not needed. It can just be:

if (versionA != null) { return versionA.CompareTo(versionB); } if (versionB != null) { return -1; } return 0;

daxian-dbw · 2017-11-07T17:43:09Z

I still have a concern about the GetHashCode() method. See my new comment at #5037 (comment)

daxian-dbw · 2017-11-07T17:48:41Z

@SteveL-MSFT This PR changes the public APIs, including:

Add a public constructions public SemanticVersion(int major, int minor, int patch, string preLabel, string buildLabel)
Remove public property Label and add two public properties PreReleaseLabel and BuildLabel.
Add a public method public static int Compare(SemanticVersion versionA, SemanticVersion versionB)

Do you think it needs the committee review?

daxian-dbw · 2017-11-07T17:55:08Z

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

+            td252.Members.Add("ToString",
+                new ScriptMethodData(@"ToString", GetScriptBlock(@"
+          $suffix = """"
+          if (![String]::IsNullOrEmpty($this.PSSemanticVersionPreLabel))


This needs to be changed if the property name is changed.

SteveL-MSFT · 2017-11-07T17:58:01Z

@daxian-dbw the general guidance is that for small api changes, the code review process is sufficient. However, any maintainer can request Committee review if needed or if the contributors for the code review can't come to an agreement.

iSazonov · 2017-11-07T18:11:25Z

SemanticVersion API is new in PowerShell Core so until RTM we can change it free if we need.

daxian-dbw · 2017-11-07T18:16:27Z

I'm fine with the API changes in this PR.

daxian-dbw

It looks to me you just simply replaced preLabel with PreReleaseLabel and didn't care about the local variables and parameter names. Local variable names and parameter names should start with a lower case letter. I will fix it for you.

daxian-dbw · 2017-11-08T20:36:56Z

.vscode/tasks.json

Please revert this change.

daxian-dbw · 2017-11-08T20:41:32Z

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

A local variable name should start with a lower-case letter.
In fact, I think you can continue to use preLabel for any non-public members.

daxian-dbw · 2017-11-08T20:42:22Z

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

Same here, I think it's OK to use preLabel here.

daxian-dbw · 2017-11-08T20:43:59Z

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

Same here. Local variable should start with lower case letter.

daxian-dbw · 2017-11-08T20:45:10Z

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

Same here. parameter names.

daxian-dbw · 2017-11-09T00:20:38Z

The test failure in AppVeyor is a known issue and will be fixed by #5379
Travis CI passed, but it's not updated here.

iSazonov · 2017-11-09T04:02:03Z

😕 Sorry, I forgot to set VS Code case-sensitive flag to search and replace.

daxian-dbw · 2017-11-09T15:41:50Z

The macOS CI was shown as passed yesterday after clicking the Travis CI "Details" link. I have no idea why it's shown as canceled today. Now we seem to have a problem with macOS CI -- it takes forever to start.

Given that I know the Travis CI previously passed, I will merge this PR.

iSazonov assigned daxian-dbw Oct 6, 2017

iSazonov requested review from BrucePay, daxian-dbw, lzybkr and vors as code owners October 6, 2017 15:36

SteveL-MSFT requested review from JamesWTruher and mirichmo October 14, 2017 03:57

daxian-dbw requested review from PaulHigin and removed request for BrucePay and vors November 1, 2017 04:25

PaulHigin suggested changes Nov 1, 2017

View reviewed changes

PaulHigin suggested changes Nov 2, 2017

View reviewed changes

iSazonov added 3 commits November 5, 2017 16:18

Make SemanticVersion compatible with SymVer 2.0

b176762

Address feedback

777628f

Replace static strings with const. Add strong checks in constructors. Add comments. Add static Compare() and fix comparisions operators.

Remove duplicate checks and add comment.

9571e06

iSazonov force-pushed the fix-semanticversion branch from 53ea5b7 to 9571e06 Compare November 5, 2017 11:20

daxian-dbw requested a review from anmenaga November 6, 2017 16:34

PaulHigin approved these changes Nov 6, 2017

View reviewed changes

daxian-dbw reviewed Nov 6, 2017

View reviewed changes

iSazonov added 4 commits November 7, 2017 07:48

Remove "s_' prefix from constant names

3654b0f

Fix comments and names.

2a7adce

Fix Compare(), GetHashCode() and add comments.

5193501

Fix tests

69e0355

SteveL-MSFT added this to the 6.0.0-RC milestone Nov 7, 2017

daxian-dbw reviewed Nov 7, 2017

View reviewed changes

iSazonov force-pushed the fix-semanticversion branch 2 times, most recently from 1f45175 to c1a9945 Compare November 8, 2017 09:22

daxian-dbw reviewed Nov 8, 2017

View reviewed changes

[Feature] Address comment and fix immature replacement

2836c8b

daxian-dbw force-pushed the fix-semanticversion branch from c1a9945 to 2836c8b Compare November 8, 2017 21:25

daxian-dbw approved these changes Nov 8, 2017

View reviewed changes

SteveL-MSFT changed the title ~~Make SemanticVersion compatible with SymVer 2.0~~ Make SemanticVersion compatible with SemVer 2.0 Nov 8, 2017

daxian-dbw merged commit c4269cb into PowerShell:master Nov 9, 2017

iSazonov deleted the fix-semanticversion branch November 9, 2017 16:05

iSazonov mentioned this pull request Nov 14, 2017

Invoke-Command -Session $s -Command {$PSVersionTable} Fails Due to SemanticVersion #1819

Closed

IISResetMe mentioned this pull request Jan 12, 2022

Revert 5.1-breaking changes to build\Start-PSBootStrap #16738

Closed

22 tasks

Conversation

iSazonov commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix description

Additional information

Uh oh!

daxian-dbw commented Nov 1, 2017

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Nov 5, 2017

Uh oh!

daxian-dbw commented Nov 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

iSazonov commented Oct 6, 2017 •

edited

Loading

daxian-dbw Nov 6, 2017 •

edited

Loading

daxian-dbw Nov 8, 2017 •

edited

Loading