Avoid redundant glob checking #10520
Conversation
| extraSrcFilesGlobResults <- mapM (checkGlobFile "." "extra-source-files") extraSrcGlobs | ||
| extraDocFilesGlobResults <- mapM (checkGlobFile "." "extra-doc-files") docGlobs | ||
| extraFilesGlobResults <- mapM (checkGlobFile "." "extra-files") extraGlobs | ||
| extraDataFilesGlobResults <- mapM (checkGlobFile rawDataDir "data-files") dataGlobs |
There was a problem hiding this comment.
Instead of re-parsing each FilePath here, we reuse the globs parsed above in checkGlob.
| -- § Missing documentation. | ||
| checkMissingDocs | ||
| extraDataFilesGlobResults | ||
| extraSrcFilesGlobResults | ||
| extraDocFilesGlobResults | ||
| extraFilesGlobResults |
There was a problem hiding this comment.
And instead of re-parsing and then re-running the glob find, we just use the results generated above.
| => FilePath -- Folder to check. | ||
| -> CabalField -- .cabal field we are checking. | ||
| -> CheckM m () | ||
| checkGlobFile cv ddir title fp = do | ||
| -> Glob -- Glob pattern. | ||
| -> CheckM m [GlobResult FilePath] |
There was a problem hiding this comment.
these args are all just String 🤪 and were incorrectly documented before.
| Nothing -> | ||
| pure [] | ||
| Just po -> do | ||
| rs <- liftCM $ runDirFileGlobM po dir parsedGlob |
There was a problem hiding this comment.
liftCM was not previously exported, but it is necessary to actually use this if the liftint interface doesn't suffice - and since that interface has a forced return of (), it can't work here.
| @@ -37,6 +37,7 @@ module Distribution.PackageDescription.Check.Monad | |||
| , checkP | |||
| , checkPkg | |||
| , liftInt | |||
| , liftCM | |||
There was a problem hiding this comment.
If we don't want to expose this I can probably replace with a function like liftInt but that allows a secondary return, like,
liftIntWith
:: (CheckInterface m -> Maybe (i m))
-> (i m -> m ([PackageCheck], r)
-> CheckM m (Maybe r)
| res <- realGlob esgs | ||
| red <- realGlob edgs | ||
| ref <- realGlob efgs | ||
| concatMap globMatches t |
There was a problem hiding this comment.
can simplify this to globMatches t and rely on callers to concat the args, too 🤷🏻
|
BTW the API change label is neither a complaint nor a barrier to getting it into 3.14; it does mean backporting to 3.12 isn't possible. |
| @@ -37,6 +37,7 @@ module Distribution.PackageDescription.Check.Monad | |||
| , checkP | |||
| , checkPkg | |||
| , liftInt | |||
| , liftCM | |||
|
So, shall we merge it? @parsonsmatt: would you like to set a merge label as per CONTRIBUTING.md and rebase/fix CI? |
|
|
This is |
|
Let me take the liberty to rebase --- maybe the problems were transient? |
|
@mergify rebase |
✅ Branch has been successfully rebased |
Mikolaj
force-pushed
the
mattp/reuse-glob-information
branch
from
350003d to
3d309d5
Compare
|
Nope, doesn't seem so. :( |
This PR modifies the glob logic to avoid doing redundant work. Results of operations are now passed forward into further checks.
Locally, this improves the time for a no-op
cabal replfrom 50s to 32s.Combined with my other PRs on this, I think this whole operation on our large
extra-source-fileswill go from 35s to 65ms 😄Template Α: This PR modifies behaviour or interface
Include the following checklist in your PR:
Template B: This PR does not modify behaviour or interface
E.g. the PR only touches documentation or tests, does refactorings, etc.
Include the following checklist in your PR: