From 5b62981b434e9aa0c3fff10c9bde5e3f3473bab7 Mon Sep 17 00:00:00 2001 From: Yvan Sraka Date: Thu, 13 Apr 2023 08:17:30 +0200 Subject: [PATCH] Apply @andreabedini code review suggestions Co-authored-by: Andrea Bedini --- app/Foliage/CmdBuild.hs | 2 +- app/Foliage/Meta.hs | 8 ++--- app/Foliage/Pages.hs | 8 ++--- app/Foliage/PreparePackageVersion.hs | 45 +++++++++++++++++++++++----- 4 files changed, 47 insertions(+), 16 deletions(-) diff --git a/app/Foliage/CmdBuild.hs b/app/Foliage/CmdBuild.hs index 670219d..a61c1e7 100644 --- a/app/Foliage/CmdBuild.hs +++ b/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 diff --git a/app/Foliage/Meta.hs b/app/Foliage/Meta.hs index 1d52505..eb2f29b 100644 --- a/app/Foliage/Meta.hs +++ b/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 diff --git a/app/Foliage/Pages.hs b/app/Foliage/Pages.hs index 8c9a501..35274df 100644 --- a/app/Foliage/Pages.hs +++ b/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 diff --git a/app/Foliage/PreparePackageVersion.hs b/app/Foliage/PreparePackageVersion.hs index fae2e39..a6bb71e 100644 --- a/app/Foliage/PreparePackageVersion.hs +++ b/app/Foliage/PreparePackageVersion.hs @@ -7,7 +7,7 @@ module Foliage.PreparePackageVersion pkgTimestamp, pkgVersionSource, pkgVersionForce, - pkgVersionDeprecated, + pkgVersionIsDeprecated, pkgDesc, sdistPath, cabalFilePath, @@ -40,7 +40,8 @@ data PreparedPackageVersion = PreparedPackageVersion pkgTimestamp :: Maybe UTCTime, pkgVersionSource :: PackageVersionSource, pkgVersionForce :: Bool, - pkgVersionDeprecated :: Bool, + pkgVersionIsDeprecated :: Bool, + pkgVersionDeprecationChanges :: [(UTCTime, Bool)], pkgDesc :: GenericPackageDescription, sdistPath :: FilePath, cabalFilePath :: FilePath, @@ -48,6 +49,33 @@ data PreparedPackageVersion = PreparedPackageVersion 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,