Semver parse fix to handle prereleases and build parts #1325

baronfel · 2016-07-21T19:02:18Z

Fixes #1324 by rewriting the prerelease and build parsing sections to adhere to spec, while maintaining the Origin, Name, and Number portions of the current logic.

This has implications for existing uses of this code, because the prior code wasn't up to spec. This code implements proper comparison for prerelease identifiers, and now discards the build portion in order to compare.

Thoughts? We hit this when a git sha that we used for stamping prerelease information on a nupkg started with a number and the semver info helper broke :(

Also touches #522 around build information.

baronfel · 2016-07-22T00:49:24Z

Also addresses #1154

forki · 2016-07-22T07:56:12Z

do we need to do the same in paket?

baronfel · 2016-07-22T15:04:17Z

Yeah, but the problem there is compounded by the fact that there are two, slightly different implementations of SemVer there already split between the bootstrapper and Paket.Core.

Is it worth splitting this out somehow and/or paket file including the source file over there?

forki · 2016-07-24T10:18:15Z

src/test/Test.FAKECore/ReleaseNotesSpecs.cs

@@ -132,7 +132,7 @@ public class when_parsing_many_pre_release_versions
 #### 2.0.0-alpha001 - December 15 2013
 * A";
        static readonly ReleaseNotesHelper.ReleaseNotes expected =
-            ReleaseNotesHelper.ReleaseNotes.New("2.0.0", "2.0.0-rc007", null, new[] { "A" }.ToFSharpList());
+            ReleaseNotesHelper.ReleaseNotes.New("2.0.0", "2.0.0-rc2", null, new[] { "A" }.ToFSharpList());


why this change?

I made this change because the comparison logic in the spec changed. rc007 is now less than rc2, so the expected output for the given release notes data had to change to take the improved comparison rules into account.

Oh that's breaking in fake. Not sure if we really want that.

On Jul 24, 2016 6:56 PM, "Chester Husk III" notifications@github.com
wrote:

In src/test/Test.FAKECore/ReleaseNotesSpecs.cs
#1325 (comment):

@@ -132,7 +132,7 @@ public class when_parsing_many_pre_release_versions

2.0.0-alpha001 - December 15 2013

A";
static readonly ReleaseNotesHelper.ReleaseNotes expected =

ReleaseNotesHelper.ReleaseNotes.New("2.0.0", "2.0.0-rc007", null, new[] { "A" }.ToFSharpList());

ReleaseNotesHelper.ReleaseNotes.New("2.0.0", "2.0.0-rc2", null, new[] { "A" }.ToFSharpList());

I made this change because the comparison logic in the spec changed. rc007
is now less than rc2, so the expected output for the given release notes
data had to change to take the improved comparison rules into account.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/fsharp/FAKE/pull/1325/files/799b4be354b7eb8da836eec694fe7385d43411d7#r71991697,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNHkwzbdlZ2itA60z_q8eYrUxUSdNks5qY5k1gaJpZM4JSHRJ
.

Current logic is wrong, though. Part of the reason I chafed working with nuget was because they implemented almost-sorta-but-not-really-semver, so none of the existing libraries worked. Do we want to do the same? Besides, if people depend on that sorting, they can either

use a prerelease of the form rc.{increment}, where increment is a number and thus compared numerically, or

use a prerelease of the form rc{padding}{number}, and be consistent, so rc007 compared to rc002,

The problem is usually the existing packages. But maybe this is less of a
problem now. It was a huge problem couple of years ago before we starter
with paket

On Jul 24, 2016 7:01 PM, "Chester Husk III" notifications@github.com
wrote:

In src/test/Test.FAKECore/ReleaseNotesSpecs.cs
#1325 (comment):

@@ -132,7 +132,7 @@ public class when_parsing_many_pre_release_versions

2.0.0-alpha001 - December 15 2013

A";
static readonly ReleaseNotesHelper.ReleaseNotes expected =

ReleaseNotesHelper.ReleaseNotes.New("2.0.0", "2.0.0-rc007", null, new[] { "A" }.ToFSharpList());

ReleaseNotesHelper.ReleaseNotes.New("2.0.0", "2.0.0-rc2", null, new[] { "A" }.ToFSharpList());

Current logic is wrong, though. Part of the reason I chafed working with
nuget was because they implemented almost-sorta-but-not-really-semver, so
none of the existing libraries worked. Do we want to do the same? Besides,
if people depend on that sorting, they can either

use a prerelease of the form rc.{increment}, where increment is a
number and thus compared numerically, or

use a prerelease of the form rc{padding}{number}, and be
consistent, so rc007 compared to rc002,

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/fsharp/FAKE/pull/1325/files/799b4be354b7eb8da836eec694fe7385d43411d7#r71991787,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNEBbyq4YdgDhIJmJc4cPCec9CYrUks5qY5pigaJpZM4JSHRJ
.

forki · 2016-07-24T10:19:38Z

yes I think I tried to split it out into a separate file but didn't suceed

baronfel · 2016-07-24T17:04:05Z

re: splitting this out - it's in a separate file with no non-framework dependencies, so we could just paket include it in paket. This could be an issue, though, as paket exposes SemVerInfo publically so I think we'd get name collisions, and Fake's SemVerInfo wouldn't be the same as Paket's.

baronfel · 2016-07-24T17:10:00Z

I think we want to have a 'correct' implementation of SemVer in Fake and Paket for internal use for sure. Then, especially in the nuget-related areas of Fake and Paket, if we need to have a stripped down version of Semver that's up to nuget spec but not SemVer proper, we can have mapping functions that assert the invariants.

baronfel · 2016-07-24T17:19:15Z

Alternatively, we could just use kolectiv's SemVer.net (FParsec parser + nice tests) in both places? https://github.com/xyncro/semver.net. That would handle the issue with sharing types around. But then we have even more back-compat issues.

baronfel · 2016-08-05T17:54:50Z

It would be very easy to leave the current implementation, mark it as obsolete, and include this new code as a 'SemVer2Helper' or something like that. But Paket is the real sticking point. Do you have any guidance here as to what you prefer?

forki · 2016-08-09T08:59:22Z

good question. I still don't know which packages would be affected

baronfel · 2016-08-19T00:35:04Z

hey, good news! Nuget client has started supporting SemVer 2.0 as of 3.5.0-rc1! https://emgarten.com/2016/08/09/semver-2-0-0-with-nuget-3-5-0-rc1/

So maybe this gives us some cover to move forward?

forki · 2016-08-22T08:19:04Z

ok let's try it.

forki · 2016-08-22T08:19:51Z

thanks!

baronfel · 2016-08-22T12:59:01Z

Want me to mirror these changes to Paket?

On Mon, Aug 22, 2016, 03:19 Steffen Forkmann notifications@github.com
wrote:

thanks!

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1325 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAjCGwuyOeRCANPa-rF-YyPDdrQNYg03ks5qiVupgaJpZM4JSHRJ
.

forki · 2016-08-22T13:00:51Z

yes. that would be awesome

Chet Husk added 4 commits July 21, 2016 13:56

fix big breaks in the semver parsing around prereleases and build parts

e881cb0

expand definition of prerelease identifier to include dash

cbc4ce2

add in handling for build identifiers according to the spec

1072131

make empty strings should result in None prerelease

799b4be

forki reviewed Jul 24, 2016
View reviewed changes

in the case of alphanumeric sorting, be explicit about ordinal sort

5fa152c

forki merged commit da55f18 into fsprojects:master Aug 22, 2016

dsyme mentioned this pull request Oct 1, 2016

SemVerHelper.PreRelease.TryParse cannot parse valid nuget pre-release identifiers #522

Closed

baronfel mentioned this pull request Oct 14, 2016

Prerelease versions with hyphen are incorrectly handled fsprojects/Paket#263

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Semver parse fix to handle prereleases and build parts #1325

Semver parse fix to handle prereleases and build parts #1325

baronfel commented Jul 21, 2016 •

edited

Loading

baronfel commented Jul 22, 2016

forki commented Jul 22, 2016

baronfel commented Jul 22, 2016

forki Jul 24, 2016

baronfel Jul 24, 2016

forki Jul 24, 2016

baronfel Jul 24, 2016

forki Jul 24, 2016

forki commented Jul 24, 2016

baronfel commented Jul 24, 2016

baronfel commented Jul 24, 2016

baronfel commented Jul 24, 2016

baronfel commented Aug 5, 2016

forki commented Aug 9, 2016

baronfel commented Aug 19, 2016

forki commented Aug 22, 2016

forki commented Aug 22, 2016

baronfel commented Aug 22, 2016

forki commented Aug 22, 2016

Semver parse fix to handle prereleases and build parts #1325

Semver parse fix to handle prereleases and build parts #1325

Conversation

baronfel commented Jul 21, 2016 • edited Loading

baronfel commented Jul 22, 2016

forki commented Jul 22, 2016

baronfel commented Jul 22, 2016

forki Jul 24, 2016

Choose a reason for hiding this comment

baronfel Jul 24, 2016

Choose a reason for hiding this comment

forki Jul 24, 2016

Choose a reason for hiding this comment

2.0.0-alpha001 - December 15 2013

baronfel Jul 24, 2016

Choose a reason for hiding this comment

forki Jul 24, 2016

Choose a reason for hiding this comment

2.0.0-alpha001 - December 15 2013

forki commented Jul 24, 2016

baronfel commented Jul 24, 2016

baronfel commented Jul 24, 2016

baronfel commented Jul 24, 2016

baronfel commented Aug 5, 2016

forki commented Aug 9, 2016

baronfel commented Aug 19, 2016

forki commented Aug 22, 2016

forki commented Aug 22, 2016

baronfel commented Aug 22, 2016

forki commented Aug 22, 2016

baronfel commented Jul 21, 2016 •

edited

Loading