remove some magic numbers from the penalty system, add a test and fix #…

…2464
fsprojects · Jul 9, 2017 · b4f4431 · b4f4431
1 parent ffc3a42
commit b4f4431
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 16 deletions.
diff --git a/src/Paket.Core/Versioning/PlatformMatching.fs b/src/Paket.Core/Versioning/PlatformMatching.fs
@@ -5,7 +5,7 @@ open ProviderImplementation.AssemblyReader.Utils.SHA1
 open Logging
 
 [<Literal>]
-let MaxPenalty = 1000000
+let MaxPenalty = 10000000
 
 type ParsedPlatformPath =
   { Name : string
@@ -49,6 +49,19 @@ let forceExtractPlatforms path =
     | None -> failwithf "Extracting platforms from path '%s' failed" path
 
 // TODO: In future work this stuff should be rewritten. This penalty stuff is more random than a proper implementation.
+// Penalty: 000000
+//               ^ Minor adjustments
+//              ^ Version jump
+//             ^ Switch between netcore -> full
+//            ^ Portable profiles
+//           ^ Unsupported Profiles
+//          ^ Fallback
+let [<Literal>] Penalty_Client = 1
+let [<Literal>] Penalty_VersionJump = 10
+let [<Literal>] Penalty_Netcore = 100
+let [<Literal>] Penalty_Portable = 1000
+let [<Literal>] Penalty_UnsupportedProfile = 10000
+let [<Literal>] Penalty_Fallback = 100000
 let rec getPlatformPenalty =
     memoize (fun (targetPlatform:TargetProfile,packagePlatform:TargetProfile) ->
         if packagePlatform = targetPlatform then
@@ -63,21 +76,21 @@ let rec getPlatformPenalty =
                 // Just check if we are compatible at all and return a high penalty
 
                 if packagePlatform.IsSupportedBy targetPlatform then
-                    700
+                    Penalty_UnsupportedProfile
                 else MaxPenalty
             | _ ->
                 let penalty =
                     targetPlatform.SupportedPlatforms
                     |> Seq.map (fun target -> getPlatformPenalty (target, packagePlatform))
                     |> Seq.append [MaxPenalty]
                     |> Seq.min
-                    |> fun p -> p + 1
+                    |> fun p -> p + Penalty_VersionJump
 
                 match targetPlatform, packagePlatform with
-                | SinglePlatform (DotNetFramework _), SinglePlatform (DotNetStandard _) -> 200 + penalty
-                | SinglePlatform (DotNetStandard _), SinglePlatform(DotNetFramework _) -> 200 + penalty
-                | SinglePlatform _, PortableProfile _ -> 500 + penalty
-                | PortableProfile _, SinglePlatform _ -> 500 + penalty
+                | SinglePlatform (DotNetFramework _), SinglePlatform (DotNetStandard _) -> Penalty_Netcore + penalty
+                | SinglePlatform (DotNetStandard _), SinglePlatform(DotNetFramework _) -> Penalty_Netcore + penalty
+                | SinglePlatform _, PortableProfile _ -> Penalty_Portable + penalty
+                | PortableProfile _, SinglePlatform _ -> Penalty_Portable + penalty
                 | _ -> penalty)
 
 let getFrameworkPenalty (fr1, fr2) =
@@ -90,12 +103,12 @@ let getPathPenalty =
         let handleEmpty () =
             match platform with
             | SinglePlatform(Native(_)) -> MaxPenalty // an empty path is considered incompatible with native targets            
-            | _ -> 2000 // an empty path is considered compatible with every .NET target, but with a high penalty so explicit paths are preferred
+            | _ -> Penalty_Fallback // an empty path is considered compatible with every .NET target, but with a high penalty so explicit paths are preferred
         match path.Platforms with
         | _ when String.IsNullOrWhiteSpace path.Name -> handleEmpty()
         | [] -> MaxPenalty // Ignore this path as it contains no platforms, but the folder apparently has a name -> we failed to detect the framework and ignore it
         | [ h ] ->
-            let additionalPen = if path.Name.EndsWith "-client" then 1 else 0
+            let additionalPen = if path.Name.EndsWith "-client" then Penalty_Client else 0
             additionalPen + getPlatformPenalty(platform,SinglePlatform h)
         | _ ->
             getPlatformPenalty(platform, TargetProfile.FindPortable path.Platforms))

diff --git a/tests/Paket.Tests/InstallModel/Penalty/PenaltySpecs.fs b/tests/Paket.Tests/InstallModel/Penalty/PenaltySpecs.fs
@@ -15,7 +15,7 @@ module ``Given a target platform`` =
     [<Test>]
     let ``it should return the right penalty for a compatible platform``() =
         getFrameworkPenalty (DotNetFramework FrameworkVersion.V4_5, DotNetFramework FrameworkVersion.V4)
-        |> shouldEqual 2
+        |> shouldEqual (Penalty_VersionJump * 2)
 
     [<Test>]
     let ``it should return > 1000 for an incompatible platform``() =
@@ -62,26 +62,26 @@ module ``Given a path`` =
         getFrameworkPathPenalty 
             [ DotNetFramework FrameworkVersion.V4
               Silverlight SilverlightVersion.V5 ] path
-        |> shouldEqual 1
+        |> shouldEqual (Penalty_VersionJump)
 
     [<Test>]
     let ``it should return the correct penalty for compatible .NET Frameworks``() =
         let path = forceExtractPlatforms "net20"
         getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V2 ] path |> shouldEqual 0
-        getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V3 ] path |> shouldEqual 1
-        getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V3_5 ] path |> shouldEqual 2
-        getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V4 ] path |> shouldEqual 3
+        getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V3 ] path |> shouldEqual Penalty_VersionJump
+        getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V3_5 ] path |> shouldEqual (Penalty_VersionJump * 2)
+        getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V4 ] path |> shouldEqual (Penalty_VersionJump * 3)
 
 module ``Given an empty path`` =
     [<Test>]
     let ``it should be okay to use from .NET``() =
         let path = forceExtractPlatforms ""
-        getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V4_5 ] path |> shouldBeSmallerThan 2000
+        getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V4_5 ] path |> shouldBeSmallerThan MaxPenalty
 
     [<Test>]
     let ``it should be okay to use from a portable profile``() =
         let path = forceExtractPlatforms ""
-        getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V4_5; Windows WindowsVersion.V8; WindowsPhoneApp WindowsPhoneAppVersion.V8_1 ] path |> shouldBeSmallerThan 2000
+        getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V4_5; Windows WindowsVersion.V8; WindowsPhoneApp WindowsPhoneAppVersion.V8_1 ] path |> shouldBeSmallerThan MaxPenalty
 
 module ``Given a list of paths`` =
     let paths =
@@ -136,6 +136,11 @@ module ``General Penalty checks`` =
     let ``best match for DotNet Standard 1.0``()=
         Paket.PlatformMatching.findBestMatch (["net20"; "net40"; "net45"; "net451"]|> List.map forceExtractPlatforms, SinglePlatform(DotNetStandard(DotNetStandardVersion.V1_0)))
         |> shouldEqual (None)
+
+    [<Test>]
+    let ``prefer net40-client over net35``()=
+        Paket.PlatformMatching.findBestMatch (["net20"; "net35"; "net40-client"]|> List.map forceExtractPlatforms, SinglePlatform(DotNetFramework(FrameworkVersion.V4_5)))
+        |> shouldEqual (Some (forceExtractPlatforms "net40-client"))
 
 
     [<Test>]