Apply @andreabedini code review suggestions

Co-authored-by: Andrea Bedini <[email protected]>

app/Foliage/CmdBuild.hs

@@ -313,7 +313,7 @@ getPreferredVersions packageVersions = Map.fromListWith (++) [(getKey p, getValu
   --                                                     ^^ TODO: replace it by `consolidateRanges`? To work with a list of `VersionRange` instead of a list of `Version`?
   where
     getKey PreparedPackageVersion {pkgId = PackageIdentifier {pkgName}} = pkgName
-    getValue PreparedPackageVersion {pkgTimestamp, pkgVersionDeprecated, pkgId = PackageIdentifier {pkgVersion}} = [(pkgTimestamp, pkgVersion) | not pkgVersionDeprecated]
+    getValue PreparedPackageVersion {pkgTimestamp, pkgVersionIsDeprecated, pkgId = PackageIdentifier {pkgVersion}} = [(pkgTimestamp, pkgVersion) | not pkgVersionIsDeprecated]
 
 -- @andreabedini points that the logic of computing the preferred versions seems in `hackage-server` to be here:
 -- https://github.com/haskell/hackage-server/blob/master/src/Distribution/Server/Features/PreferredVersions/State.hs#L39-L44

app/Foliage/Meta.hs

@@ -26,7 +26,7 @@ module Foliage.Meta
     revisionNumber,
     DeprecationSpec (DeprecationSpec),
     deprecationTimestamp,
-    isDeprecated,
+    deprecationIsDeprecated,
     PackageVersionSource,
     pattern TarballSource,
     pattern GitHubSource,
@@ -178,7 +178,7 @@ data PackageVersionSpec = PackageVersionSpec
     packageVersionSource :: PackageVersionSource,
     -- | revisions
     packageVersionRevisions :: [RevisionSpec],
-    -- | deprpecations
+    -- | deprecations
     packageVersionDeprecations :: [DeprecationSpec],
     -- | force version
     packageVersionForce :: Bool
@@ -223,7 +223,7 @@ revisionMetaCodec =
 
 data DeprecationSpec = DeprecationSpec
   { deprecationTimestamp :: UTCTime,
-    isDeprecated :: Bool
+    deprecationIsDeprecated :: Bool
   }
   deriving (Show, Eq, Generic)
   deriving anyclass (Binary, Hashable, NFData)
@@ -234,7 +234,7 @@ deprecationMetaCodec =
     <$> timeCodec "timestamp"
     .= deprecationTimestamp
     <*> withDefault True (Toml.bool "deprecated")
-    .= isDeprecated
+    .= deprecationIsDeprecated
 
 timeCodec :: Toml.Key -> TomlCodec UTCTime
 timeCodec key = Toml.dimap (utcToZonedTime utc) zonedTimeToUTC $ Toml.zonedTime key

app/Foliage/Pages.hs

@@ -117,7 +117,7 @@ makeAllPackageVersionsPage currentTime outputDir packageVersions =
     entries =
       -- collect all cabal file revisions including the original cabal file
       foldMap
-        ( \PreparedPackageVersion {pkgId, pkgTimestamp, pkgVersionSource, pkgVersionDeprecated, cabalFileRevisions} ->
+        ( \PreparedPackageVersion {pkgId, pkgTimestamp, pkgVersionSource, pkgVersionIsDeprecated, cabalFileRevisions} ->
             -- original cabal file
             AllPackageVersionsPageEntryPackage
               { allPackageVersionsPageEntryPkgId = pkgId,
@@ -126,7 +126,7 @@ makeAllPackageVersionsPage currentTime outputDir packageVersions =
                 allPackageVersionsPageEntrySource = pkgVersionSource,
                 -- FIXME: this weirdly seems to not work (display a `Deprecated` badge on all package version page) ...
                 -- don't understand yet why! :/
-                allPackageVersionsPageEntryDeprecated = pkgVersionDeprecated
+                allPackageVersionsPageEntryDeprecated = pkgVersionIsDeprecated
               }
               -- list of revisions
               : [ AllPackageVersionsPageEntryRevision
@@ -142,7 +142,7 @@ makeAllPackageVersionsPage currentTime outputDir packageVersions =
         & sortOn (Down . allPackageVersionsPageEntryTimestamp)
 
 makePackageVersionPage :: FilePath -> PreparedPackageVersion -> Action ()
-makePackageVersionPage outputDir PreparedPackageVersion {pkgId, pkgTimestamp, pkgVersionSource, pkgDesc, cabalFileRevisions, pkgVersionDeprecated} = do
+makePackageVersionPage outputDir PreparedPackageVersion {pkgId, pkgTimestamp, pkgVersionSource, pkgDesc, cabalFileRevisions, pkgVersionIsDeprecated} = do
   traced ("webpages / package / " ++ prettyShow pkgId) $ do
     IO.createDirectoryIfMissing True (outputDir </> "package" </> prettyShow pkgId)
     TL.writeFile (outputDir </> "package" </> prettyShow pkgId </> "index.html") $
@@ -152,7 +152,7 @@ makePackageVersionPage outputDir PreparedPackageVersion {pkgId, pkgTimestamp, pk
             "cabalFileRevisions" .= map fst cabalFileRevisions,
             "pkgDesc" .= jsonGenericPackageDescription pkgDesc,
             "pkgTimestamp" .= pkgTimestamp,
-            "pkgVersionDeprecated" .= pkgVersionDeprecated
+            "pkgVersionDeprecated" .= pkgVersionIsDeprecated
           ]
 
 indexPageTemplate :: Template

app/Foliage/PreparePackageVersion.hs

@@ -7,7 +7,7 @@ module Foliage.PreparePackageVersion
         pkgTimestamp,
         pkgVersionSource,
         pkgVersionForce,
-        pkgVersionDeprecated,
+        pkgVersionIsDeprecated,
         pkgDesc,
         sdistPath,
         cabalFilePath,
@@ -40,14 +40,42 @@ data PreparedPackageVersion = PreparedPackageVersion
     pkgTimestamp :: Maybe UTCTime,
     pkgVersionSource :: PackageVersionSource,
     pkgVersionForce :: Bool,
-    pkgVersionDeprecated :: Bool,
+    pkgVersionIsDeprecated :: Bool,
+    pkgVersionDeprecationChanges :: [(UTCTime, Bool)],
     pkgDesc :: GenericPackageDescription,
     sdistPath :: FilePath,
     cabalFilePath :: FilePath,
     originalCabalFilePath :: FilePath,
     cabalFileRevisions :: [(UTCTime, FilePath)]
   }
 
+-- @andreabedini comments:
+--
+-- The function `preparePackageVersion` has a bit of a special role which I
+-- should comment upon.
+--
+-- There are at three sources of information about a package:
+--
+--   * the path of the meta file: `_sources/pkg-name/pkg-version/meta.toml`
+--   * the content of `meta.toml`
+--   * the tarball/sdist pointed by `meta.toml`
+--
+--
+-- Before #37 I used to refer to these three pieces of data independently,
+-- thinking it would be a good idea to keep the data-pipeline granular.
+--
+-- While working on #37, I realised this granularity was leading me to have
+-- consistency checks scattered around the code so I figured it would make more
+-- sense to centralise these checks into a single function and to use a type
+-- (`PreparedPackageVersion`) as evidence that everything is consistent (e.g.
+-- the package name inferred from the meta.toml path is the same as the one in
+-- the cabal file of the source distribution).
+--
+-- This function has also the chance to denormalise some data (i.e. repeating it
+-- multiple times in different forms) for easy consumption downstream. This
+-- could be split out in the future if `PreparedPackageVersion` starts to become
+-- a kitchen sink.
+
 preparePackageVersion :: FilePath -> FilePath -> Action PreparedPackageVersion
 preparePackageVersion inputDir metaFile = do
   let (name, version) = case splitDirectories metaFile of
@@ -120,14 +148,16 @@ preparePackageVersion inputDir metaFile = do
             | RevisionSpec {revisionTimestamp, revisionNumber} <- packageVersionRevisions pkgSpec
           ]
 
-  let lastDeprecation =
+  -- FIXME here is where we chack that there are no "double deprecations" (i.e. two consecutive (in time) `deprecated = true` or `deprecated = false`)
+  let pkgVersionDeprecationChanges =
         sortOn
           (Down . fst)
-          [ (deprecationTimestamp, isDeprecated)
-            | DeprecationSpec {deprecationTimestamp, isDeprecated} <- packageVersionDeprecations pkgSpec
+          [ (deprecationTimestamp, deprecationIsDeprecated)
+            | DeprecationSpec {deprecationTimestamp, deprecationIsDeprecated} <- packageVersionDeprecations pkgSpec
           ]
 
-  let isDeprecated = case lastDeprecation of
+  let lastDeprecation = [] -- TODO
+  let pkgVersionIsDeprecated = case lastDeprecation of
         ((_, x):_) -> x
         [] -> False
 
@@ -137,7 +167,8 @@ preparePackageVersion inputDir metaFile = do
         pkgTimestamp = packageVersionTimestamp pkgSpec,
         pkgVersionSource = packageVersionSource pkgSpec,
         pkgVersionForce = packageVersionForce pkgSpec,
-        pkgVersionDeprecated = isDeprecated,
+        pkgVersionDeprecationChanges,
+        pkgVersionIsDeprecated,
         pkgDesc,
         sdistPath,
         cabalFilePath,