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
There was a problem hiding this comment.
arsedComponents.Length == 0 [](start = 17, length = 28)
i don't know if this condition can exist. Even [string]::Empty.Split('.').Length will return 1
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
Now I see what you are referring to. I'll remove that part.
There was a problem hiding this comment.
is there a reason why [SemanticVersion]::new() is not supported? It's supported for System.Version
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Under what scenario would we want a default constructor? If you want 0.0.0, be explicit.
There was a problem hiding this comment.
For now, let's error on the side of simplicity and we can revisit this if there's customer demand for it
lzybkr
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We should not use default arguments in public apis. The guidance is to provide multiple overloads for languages that may not support default arguments.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Under what scenario would we want a default constructor? If you want 0.0.0, be explicit.
7b4f3a5 to
5b032ff
Compare
|
Regarding the semver.org spec, we are not diverging. The resulting semver is conformant to semver. |
|
@lzybkr I believe I've addressed all of your concerns |
|
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 1If 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. |
|
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. |
|
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. |
|
I've opened semver/semver#368 |
|
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
left a comment
There was a problem hiding this comment.
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.
5b032ff to
a72163a
Compare
|
@lzybkr all fixed |
[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