Skip to content
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

Semver parse fix to handle prereleases and build parts #1325

Merged
merged 5 commits into from
Aug 22, 2016
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 99 additions & 36 deletions src/app/FakeLib/SemVerHelper.fs
Original file line number Diff line number Diff line change
Expand Up @@ -4,28 +4,75 @@ module Fake.SemVerHelper
open System
open System.Text.RegularExpressions

let identRE = Regex("[0-9A-Za-z-]+", RegexOptions.Compiled)
[<CustomEquality;CustomComparison>]
type Ident =
| AlphaNumeric of string | Numeric of int64
override x.Equals(yobj) =
match yobj with
| :? Ident as y ->
match x,y with
| AlphaNumeric a, AlphaNumeric b -> a = b
| AlphaNumeric _, Numeric _ -> false
| Numeric _, AlphaNumeric _ -> false
| Numeric a, Numeric b -> a = b
| _ -> false
override x.ToString() = match x with | AlphaNumeric a -> a | Numeric b -> string b
override x.GetHashCode() = match x with | AlphaNumeric a -> hash a | Numeric b -> hash b
interface IComparable with
member x.CompareTo yobj =
match yobj with
// spec says that alpha is always greater than numeric, alpha segments are compared lexicographically
| :? Ident as y ->
match x,y with
| AlphaNumeric a, AlphaNumeric b -> compare a b
| AlphaNumeric _, Numeric _ -> 1
| Numeric _, AlphaNumeric _ -> -1
| Numeric a, Numeric b -> compare a b
| _ -> invalidArg "yobj" "cannot compare values of different types"

let parseIdent s =
match Int64.TryParse s with
| true, i -> Numeric i
| false, _ -> AlphaNumeric s

[<CustomEquality; CustomComparison>]
type PreRelease =
{ Origin: string
Name: string
Number: int option }
Number: int option
Parts : Ident list }
static member TryParse str =
let m = Regex("^(?<name>[a-zA-Z]+)(?<number>\d*)$").Match(str)
match m.Success, m.Groups.["name"].Value, m.Groups.["number"].Value with
| true, name, "" -> Some { Origin = str; Name = name; Number = None }
| true, name, number -> Some { Origin = str; Name = name; Number = Some (int number) }
| _ -> None
let idents = splitRemove '.' str
match idents |> List.forall (identRE.IsMatch) with
| false -> None
| true ->
let namedPrereleaseRE = Regex("^(?<name>[a-zA-Z]+)(?<number>\d*)$", RegexOptions.Compiled)
let m = namedPrereleaseRE.Match(str)
let parts = idents |> List.map parseIdent
match m.Success, m.Groups.["name"].Value, m.Groups.["number"].Value with
| true, name, "" -> Some { Origin = str; Name = name; Number = None; Parts = parts }
| true, name, number -> Some { Origin = str; Name = name; Number = Some (int number); Parts = parts }
| false, _, _ when idents.Length = 1 ->
Some { Origin = idents.[0]; Name = ""; Parts = parts; Number = match Int32.TryParse idents.[0] with | true, n -> Some n | false, _ -> None; }
| false, _, _ ->
Some { Origin = str; Name = ""; Number = None; Parts = parts}
override x.ToString() = String.Join(".", x.Parts |> List.map string)
override x.Equals(yobj) =
match yobj with
| :? PreRelease as y -> x.Origin = y.Origin
| _ -> false
override x.GetHashCode() = hash x.Origin
interface System.IComparable with
interface IComparable with
member x.CompareTo yobj =
match yobj with
// spec says that longer prereleases are bigger
| :? PreRelease as y ->
if x.Name <> y.Name then compare x.Name y.Name else
compare x.Number y.Number
if x.Parts.Length <> y.Parts.Length then compare x.Parts.Length y.Parts.Length
else
List.zip x.Parts y.Parts
|> List.fold (fun cmp (x,y) -> if cmp <> 0 then cmp else compare x y) 0

| _ -> invalidArg "yobj" "cannot compare values of different types"

/// Contains the version information.
Expand All @@ -40,14 +87,14 @@ type SemVerInfo =
/// The optional PreRelease version
PreRelease : PreRelease option
/// The optional build no.
Build: string }
Build: string
BuildIdentifiers : Ident list}
override x.ToString() =
sprintf "%d.%d.%d" x.Major x.Minor x.Patch +
(match x.PreRelease, isNotNullOrEmpty x.Build with
| Some preRelease, _ -> "-" + preRelease.Name
| None, true -> "-"
| _ -> "") +
(if isNotNullOrEmpty x.Build then "." + x.Build else "")
sprintf "%d.%d.%d%s%s" x.Major x.Minor x.Patch
(match x.PreRelease with
| Some preRelease -> "-" + string preRelease
| None -> "")
(if isNotNullOrEmpty x.Build then "+" + x.Build else "")

override x.Equals(yobj) =
match yobj with
Expand All @@ -57,7 +104,7 @@ type SemVerInfo =
| _ -> false

override x.GetHashCode() = hash (x.Minor,x.Minor,x.Patch,x.PreRelease,x.Build)
interface System.IComparable with
interface IComparable with
member x.CompareTo yobj =
match yobj with
| :? SemVerInfo as y ->
Expand All @@ -67,11 +114,8 @@ type SemVerInfo =
if x.PreRelease = y.PreRelease && x.Build = y.Build then 0 else
if x.PreRelease.IsNone && x.Build = "" then 1 else
if y.PreRelease.IsNone && y.Build = "" then -1 else
if x.PreRelease <> y.PreRelease then compare x.PreRelease y.PreRelease else
if x.Build <> y.Build then
match Int32.TryParse x.Build, Int32.TryParse y.Build with
| (true,b1),(true,b2) -> compare b1 b2
| _ -> compare x.Build y.Build
if x.PreRelease <> y.PreRelease then compare x.PreRelease y.PreRelease
// spec says that build should have no impact on comparisons
else
0
| _ -> invalidArg "yobj" "cannot compare values of different types"
Expand All @@ -91,23 +135,42 @@ let isValidSemVer input =
///
/// parse "1.0.0-rc.1" < parse "1.0.0" // true
/// parse "1.2.3-alpha" > parse "1.2.2" // true
/// parse "1.2.3-alpha2" > parse "1.2.3-alpha" // true
/// parse "1.2.3-alpha002" > parse "1.2.3-alpha1" // true
/// parse "1.5.0-beta.2" > parse "1.5.0-rc.1" // false
/// parse "1.2.3-alpha2" > parse "1.2.3-alpha" // true, but only because of lexical compare
/// parse "1.2.3-alpha002" > parse "1.2.3-alpha1" // false, due to lexical compare
/// parse "1.5.0-beta.2" > parse "1.5.0-rc.1" // false, due to lexical compare of first prerelease identitifer
/// parse "1.5.0-beta.2" > parse "1.5.0-beta.3" // true, due to numeric compare of second prerelease identitifer
/// parse "1.5.0-0123.001" < parse "1.5.0-0123.002" // true, due to numeric compare of second prerelease identifier
/// parse "1.0.0+lol" = parse "1.0.0" // true, because build identifiers do not influence comparison
let parse version =
let splitted = split '.' version
let l = splitted.Length
let patch,preRelease =
if l <= 2 then 0,"" else
let splitted' = split '-' splitted.[2]
Int32.Parse splitted'.[0], if splitted'.Length > 1 then splitted'.[1] else ""

let startPos (c : char) (s :string) = match s.IndexOf(c) with | -1 -> None | n -> Some n
let buildPartStart = startPos '+' version
let prereleasePartStart = startPos '-' version
let main, pre, build =
match prereleasePartStart, buildPartStart with
| None, None -> version, None, None
| Some n, None -> version.[0..n-1], Some version.[n+1..], None
| None, Some n -> version.[0..n-1], None, Some version.[n+1..]
| Some n, Some m -> version.[0..n-1], Some version.[n+1..m-1], Some version.[m+1..]

let maj, minor, patch =
match split '.' main with
// odd case for sillies that use non-standard semver? that's why xs instead of []
| maj::min::pat::xs -> Int32.Parse maj, Int32.Parse min, Int32.Parse pat
| maj::min::[] -> Int32.Parse maj, Int32.Parse min, 0
| maj::[] -> Int32.Parse maj, 0, 0
| [] -> 0,0,0
| _ -> failwith "unknown semver format"


let buildParts = Option.map (fun b -> splitRemove '.' b) build
if buildParts.IsSome && buildParts.Value |> List.exists (not << identRE.IsMatch) then failwith "unknown semver build format"

{ Major = if l > 0 then Int32.Parse splitted.[0] else 0
Minor = if l > 1 then Int32.Parse splitted.[1] else 0
{ Major = maj
Minor = minor
Patch = patch
PreRelease = PreRelease.TryParse preRelease
Build = if l > 3 then splitted.[3] else ""
PreRelease = Option.bind PreRelease.TryParse pre
Build = defaultArg build ""
BuildIdentifiers = defaultArg (buildParts |> Option.map (List.map parseIdent)) List.empty
}


Expand Down
2 changes: 2 additions & 0 deletions src/app/FakeLib/StringHelper.fs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ let inline trimSlash (s : string) = s.TrimEnd('\\')
/// Splits the given string at the given char delimiter
let inline split (delimiter : char) (text : string) = text.Split [| delimiter |] |> Array.toList

let inline splitRemove (delimiter : char) (text : string) = text.Split ([|delimiter|], StringSplitOptions.RemoveEmptyEntries) |> Array.toList

/// Splits the given string at the given string delimiter
let inline splitStr (delimiterStr : string) (text : string) =
text.Split([| delimiterStr |], StringSplitOptions.None) |> Array.toList
Expand Down
2 changes: 1 addition & 1 deletion src/test/Test.FAKECore/ReleaseNotesSpecs.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Member

Choose a reason for hiding this comment

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

why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

On Jul 24, 2016 6:56 PM, "Chester Husk III" [email protected]
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
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

  1. use a prerelease of the form rc.{increment}, where increment is a number and thus compared numerically, or
  2. use a prerelease of the form rc{padding}{number}, and be consistent, so rc007 compared to rc002,

Copy link
Member

Choose a reason for hiding this comment

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

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" [email protected]
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

  1. use a prerelease of the form rc.{increment}, where increment is a
    number and thus compared numerically, or
  2. 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
.


It should_parse =
() => ReleaseNotesHelper.parseReleaseNotes(Notes.FromString(input)).ShouldEqual(expected);
Expand Down
12 changes: 6 additions & 6 deletions src/test/Test.FAKECore/SemVerHelperSpecs.cs
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,13 @@ public class when_validating_semver_strings
public class when_parsing_semver_strings
{
static SemVerHelper.SemVerInfo semVer;
Because of = () => semVer = SemVerHelper.parse("1.2.3-alpha.beta");
Because of = () => semVer = SemVerHelper.parse("1.2.3-alpha+beta");

It should_parse_major = () => semVer.Major.ShouldEqual(1);
It should_parse_minor = () => semVer.Minor.ShouldEqual(2);
It should_parse_patch = () => semVer.Patch.ShouldEqual(3);
It should_parse_prerelease = () => semVer.PreRelease.ShouldEqual(
FSharpOption<SemVerHelper.PreRelease>.Some(new SemVerHelper.PreRelease("alpha", "alpha", FSharpOption<int>.None)));
FSharpOption<SemVerHelper.PreRelease>.Some(new SemVerHelper.PreRelease("alpha", "alpha", FSharpOption<int>.None, new[] { SemVerHelper.Ident.NewAlphaNumeric("alpha") }.ToFSharpList())));
It should_parse_build = () => semVer.Build.ShouldEqual("beta");
}

Expand Down Expand Up @@ -96,9 +96,9 @@ public class when_comparing_semvers
() => SemVerHelper.parse("1.0.0-alpha.1")
.ShouldBeLessThan(SemVerHelper.parse("1.0.0-alpha.beta"));

It should_assume_alpha_is_smaller_tha_beta =
It should_assume_that_longer_prereleases_are_greater =
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to keep the old tests as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept the test data, but per the spec for these inputs 1.0.0-beta is larger, because a shorter prerelease tag is bigger than a longer prerelease tag.

() => SemVerHelper.parse("1.0.0-alpha.beta")
.ShouldBeLessThan(SemVerHelper.parse("1.0.0-beta"));
.ShouldBeGreaterThan(SemVerHelper.parse("1.0.0-beta"));

It should_assume_empty_build_no_in_beta_build_is_smaller_than_text_in_build =
() => SemVerHelper.parse("1.0.0-beta")
Expand Down Expand Up @@ -128,9 +128,9 @@ public class when_comparing_semvers
() => SemVerHelper.parse("2.3.4-alpha2")
.ShouldBeGreaterThan(SemVerHelper.parse("2.3.4-alpha"));

It should_assume_alpha003_is_greater_than_alpha2 =
It should_assume_alpha003_is_less_than_alpha2_because_lexicalsort =
() => SemVerHelper.parse("2.3.4-alpha003")
.ShouldBeGreaterThan(SemVerHelper.parse("2.3.4-alpha2"));
.ShouldBeLessThan(SemVerHelper.parse("2.3.4-alpha2"));

It should_assume_rc_is_greater_than_beta2 =
() => SemVerHelper.parse("2.3.4-rc")
Expand Down