Allow [SemanticVersion] to be constructed with just major or major+minor #3696

SteveL-MSFT · 2017-05-04T18:13:17Z

[SemanticVersion]::new() required major, minor, and patch to be specfied which broke scenarios like:

$psversiontable.psversion -gt "3.0" which is used by PowerShellGet to determine if a module is compatible with the current version of PowerShell.

Change is to allow specifying only major or major+minor where the missing segments default to zero.

Addresses #1618

JamesWTruher · 2017-05-04T20:34:12Z

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

@@ -544,24 +544,24 @@ private static bool TryParseVersion(string version, ref VersionResult result)

            var versionSansLabel = (dashIndex < 0) ? version : version.Substring(0, dashIndex);
            string[] parsedComponents = versionSansLabel.Split(Utils.Separators.Dot);
-            if (parsedComponents.Length != 3)
+            if (parsedComponents.Length == 0 || parsedComponents.Length > 3)


arsedComponents.Length == 0 [](start = 17, length = 28)

i don't know if this condition can exist. Even [string]::Empty.Split('.').Length will return 1

I believe that's the ETS .Length returning 1 where we treat scalars as arrays and add both .Length and .Count properties. If you do [string]::Empty.Split('.')[0].Length, you get 0 as expected

This is C# - PowerShell ETS is not relevant, and you are inspecting the length of the first element, not the number of elements in the array. msdn does say:

If this instance does not contain any of the characters in separator, the returned array consists of a single element that contains this instance.

So it's a one element array that contains an empty string. In any case, there is already code that causes an empty string to fail before the split() statement.

@JamesWTruher pointed out that the length can't be 0 and that part of the test should be removed. I agreed. Are saying that it is possible?

Now I see what you are referring to. I'll remove that part.

JamesWTruher · 2017-05-04T20:50:59Z

test/powershell/engine/SemanticVersion.Tests.ps1

@@ -178,10 +199,6 @@ Describe "SemanticVersion api tests" -Tags 'CI' {
            # Revision isn't supported
            { [SemanticVersion]::new([Version]::new(0, 0, 0, 4)) } | Should Throw # PSArgumentException
            { [SemanticVersion]::new([Version]::new("1.2.3.4")) } | Should Throw # PSArgumentException
-
-            # Build is required


is there a reason why [SemanticVersion]::new() is not supported? It's supported for System.Version

I don't know if that was an explicit decision or accidentally left out. I don't see any reason why it shouldn't be supported as it seems 0.0.0 is a valid semver. I'll add support since it's a small change even if not related to this change :)

Under what scenario would we want a default constructor? If you want 0.0.0, be explicit.

For now, let's error on the side of simplicity and we can revisit this if there's customer demand for it

lzybkr

Note that the original implementation matched the specification, specifically: http://semver.org/#spec-item-2

We may want to discuss how best to diverge from the specification in a way that the community finds acceptable.

lzybkr · 2017-05-04T21:34:29Z

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

@@ -413,7 +413,7 @@ public SemanticVersion(int major, int minor, int patch, string label)
        /// <exception cref="PSArgumentException">
        /// If <paramref name="major"/>, <paramref name="minor"/>, or <paramref name="patch"/> is less than 0.
        /// </exception>
8000
;
-        public SemanticVersion(int major, int minor, int patch)
+        public SemanticVersion(int major, int minor = 0, int patch = 0)


We should not use default arguments in public apis. The guidance is to provide multiple overloads for languages that may not support default arguments.

Will change

lzybkr · 2017-05-04T21:35:39Z

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

@@ -544,24 +544,24 @@ private static bool TryParseVersion(string version, ref VersionResult result)

            var versionSansLabel = (dashIndex < 0) ? version : version.Substring(0, dashIndex);
            string[] parsedComponents = versionSansLabel.Split(Utils.Separators.Dot);
-            if (parsedComponents.Length != 3)
+            if (parsedComponents.Length == 0 || parsedComponents.Length > 3)


This is C# - PowerShell ETS is not relevant, and you are inspecting the length of the first element, not the number of elements in the array. msdn does say:

If this instance does not contain any of the characters in separator, the returned array consists of a single element that contains this instance.

lzybkr · 2017-05-04T21:41:31Z

test/powershell/engine/SemanticVersion.Tests.ps1

@@ -178,10 +199,6 @@ Describe "SemanticVersion api tests" -Tags 'CI' {
            # Revision isn't supported
            { [SemanticVersion]::new([Version]::new(0, 0, 0, 4)) } | Should Throw # PSArgumentException
            { [SemanticVersion]::new([Version]::new("1.2.3.4")) } | Should Throw # PSArgumentException
-
-            # Build is required


Under what scenario would we want a default constructor? If you want 0.0.0, be explicit.

SteveL-MSFT · 2017-05-04T22:30:55Z

Regarding the semver.org spec, we are not diverging. The resulting semver is conformant to semver.

SteveL-MSFT · 2017-05-04T23:01:53Z

@lzybkr I believe I've addressed all of your concerns

lzybkr · 2017-05-04T23:35:55Z

I disagree - the bnf for parsing a semantic version is pretty clear - 3 parts: https://github.com/mojombo/semver/blob/master/semver.md

I found another example where we aren't matching the spec:

PS> [System.Management.Automation.SemanticVersion]"01.01.01"

Major Minor Patch Label
----- ----- ----- -----
    1     1     1

If we're more permissive than the spec, people will rely on it, and where those uses occur, we can expect issues with more conforming tools.

SteveL-MSFT · 2017-05-05T00:05:08Z

Leading zeros aren't allowed so the resulting semver conforms. I guess the question is whether the resulting object is what matters for conformance or the input to the constructor. Seems you prefer a more strict approach and reject 01.0.0 vs producing a valid semver. This also means that you can't compare a semver like psversion against "3.0" since the latter is not a semver.

lzybkr · 2017-05-05T00:10:38Z

Comparing with System.Version is no problem, but comparing with "3.0" might be, based on my reading of the spec.

It might be worth opening an issue in the repo I linked to.

SteveL-MSFT · 2017-05-05T00:18:13Z

I've opened semver/semver#368

SteveL-MSFT · 2017-05-05T04:46:46Z

The response I got from the maintainer of semver indicates it is reasonable and not explicitly defined by semver spec. Also, this thread semver/semver#344 has supporters for both sides with no official stance, but examples of other libraries that have taken both approaches (strict and loose). I think it's reasonable to allow constructing a semver with defaults to zero. Cc @PowerShell/powershell-committee

lzybkr

OK - changes look fine except for what looks like an unnecessary test.

…fied which broke scenarios like: `$psversiontable.psversion -gt "3.0"` which is used by PowerShellGet to determine if a module is compatible with the current version of PowerShell. Change is to allow specifying only major or major+minor where the missing segments default to zero by providing overloaded constructors and allow the string parsing method to not require major, minor, and patch segments to all be specified (only major is required). Based on the [response](semver/semver#368) from the maintainer of semver, there is no requirement to have strict conformance for the inputs to the constructor and allowing "3.0" to result in a semver of 3.0.0 is reasonable.

SteveL-MSFT · 2017-05-08T16:55:36Z

@lzybkr all fixed

SteveL-MSFT added this to the 6.0.0-beta1 milestone May 4, 2017

SteveL-MSFT assigned mirichmo May 4, 2017

SteveL-MSFT requested a review from JamesWTruher May 4, 2017 18:13

msftclas added the cla-not-required label May 4, 2017

JamesWTruher reviewed May 4, 2017

View reviewed changes

lzybkr suggested changes May 4, 2017

View reviewed changes

SteveL-MSFT force-pushed the semanticversion branch from 7b4f3a5 to 5b032ff Compare May 4, 2017 22:29

lzybkr reviewed May 8, 2017

View reviewed changes

SteveL-MSFT force-pushed the semanticversion branch from 5b032ff to a72163a Compare May 8, 2017 16:01

SteveL-MSFT assigned lzybkr and unassigned mirichmo May 8, 2017

lzybkr approved these changes May 8, 2017

View reviewed changes

lzybkr merged commit 6342944 into PowerShell:master May 8, 2017

lzybkr mentioned this pull request May 8, 2017

System.Management.Automation.SemanticVersion should support more string constructors #1618

Closed

nightroman mentioned this pull request May 15, 2017

[SemanticVersion] comparison with [Version] constructed with just major+minor #3786

Closed

SteveL-MSFT deleted the semanticversion branch June 19, 2017 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow [SemanticVersion] to be constructed with just major or major+minor #3696

Allow [SemanticVersion] to be constructed with just major or major+minor #3696

Allow [SemanticVersion] to be constructed with just major or major+minor #3696

Allow [SemanticVersion] to be constructed with just major or major+minor #3696

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