Log a warning for missing dependency versions

[JoelLefkowitz]

Description of the change

Fixes #1313 by adding a warning message when the installed versions of packages are outside the specified dependency ranges.

Checklist:

• Added the change to the "Unreleased" section of the changelog
• Added some example of the new feature to the README
• Added a test for the contribution (if applicable)

Tests

Adds a test spec: test/Spago/Install.purs > Spec.it "warns when specified dependency versions do not exist" that creates a workspace and adds some packages with ranges that exceed the registry's versions. Asserts that a warning message is shown.

Implementation

Adds a type VersionResolution to map the requested to the resolved versions for each package:

type VersionResolution =
  { name :: PackageName
  , requested :: Range
  , resolved :: Version
  }

resolutions :: Map PackageName Package -> Map PackageName Range -> Array VersionResolution
resolutions registry dependencies = do
  Tuple name requested <- toUnfoldable dependencies
  case (lookup name registry) of
    Just (RegistryVersion resolved) -> [ { name, requested, resolved } ]
    _ -> []

In src/Spago/Command/Fetch.purs the versions are resolved and any missing versions log a warning:

PackageSetBuild _info set -> do
  packages <- depsRanges # onEachEnvM \depsRanges' ->
    getTransitiveDepsFromPackageSet set $ (Array.fromFoldable $ Map.keys depsRanges')

  let
    versions = resolutions (mergeEnvs packages) (mergeEnvs depsRanges)
    missing = Array.filter unavailable versions

  unless (Array.null missing) (logWarn $ missingWarning missing)
  pure packages

[JoelLefkowitz]

Hey @f-f could you please approve the workflows to run?

[JoelLefkowitz]

Hey @f-f thank you for the detailed feedback, I've made the changes you have suggested.

Summary of changes:

• Removed the error description from the README
• Changed packages versions to package versions
• Changed unless (Array.null missing) to when (Array.length missing > 0)
• Renamed resolutions to resolvePackageVersionsToRanges
• Inlined mergeEnv into getWorkspacePackageDeps
• Renamed withDependencies to insertConfigDependencies and moved it into the test module
• Removed the Resolution module and inlined its contents into getWorkspacePackageDeps
• Used map instead of bind in resolvePackageVersionsToRanges

I have some questions about the style and conventions that you'd reccommend for purescript and I'd really appreciate your advice:

I always find the monad instance of Array confusing - could we rewrite this function e.g. with map?

Is your preference to use map over bind only when returning zero/one element(s) each time? If we wanted to return multiple elements at a time do you think it is more readable to use map and join instead of bind?

Since this stuff is only used in one place I would like to inline it in the let block in line 629 in Spago.Command.Fetch

Would you also inline a large function that is only used once? For example resolvePackageVersionsToRanges could also be inlined into the getWorkspacePackageDeps function too but its function signature helps explain what it does Map PackageName Package -> Map PackageName Range -> Array VersionResolution.

What do you think about a function like mergeEnv that needs a type annotation when inside a let block to enable polymorphism? From what I've seen type annotations don't usually appear in let blocks.

More self-documenting name for this function:
Suggested change
versions = resolutions (mergeEnvs packages) (mergeEnvs depsRanges)
versions = resolvePackageVersionsToRanges (mergeEnvs packages) (mergeEnvs depsRanges)

I have seen in some of the prelude libraries that function names are shadowed so they can be combined with named module imports. For example in the Array and NonEmptyArray modules they each define a length function instead of using unique names like arrayLength and nonEmptyArrayLength. I assume this is because Array.length is cleaner to read than arrayLength. Are there any guidelines on when to follow this pattern and when it is preferable to write something like VersionResolution.resolutions over resolvePackageVersionsToRanges?

[f-f]

Lovely! Left a few more style suggestions but otherwise this is good to merge

[f-f]

We have facilities for logging on multiple lines: logWarn (and the rest of the logging functions) take a Loggable a, which includes arrays.

So the next function could read like this:

Suggested change

	let list k v = " - " <> PackageName.print k <> ": " <> v
	logWarn <<< joinWith "\n" $ join
	[ [ "The following package versions do not exist in your package set:" ]
	, (\{ name, requested } -> list name $ Range.print requested) <$> missing
	, [ "", "Proceeding with the latest available versions instead:" ]
	, (\{ name, resolved } -> list name $ Version.print resolved) <$> missing
	]
	let listItem k v = toDoc $ " - " <> PackageName.print k <> ": " <> v
	logWarn $
	[ toDoc "The following package versions do not exist in your package set:" ]
	<> map (\{ name, requested } -> list name $ Range.print requested) missing
	[ Log.break, toDoc "Proceeding with the latest available versions instead:" ]
	<> map (\{ name, resolved } -> list name $ Version.print resolved) missing

(I didn't test this code so might not be exactly right, but you'll find this pattern all over the codebase if you search for toDoc)

[f-f]

This can be condensed with mapMaybeWithKey (and let's move it to the let block as well):

Suggested change

	resolvePackageVersionsToRanges registry = Map.toUnfoldable >>> Array.foldl
	( \acc (Tuple name requested) ->
	case (Map.lookup name registry) of
	Just (RegistryVersion resolved) -> Array.snoc acc { name, requested, resolved }
	_ -> acc
	)
	[]
	resolvePackageVersionsToRanges registry = Array.fromFoldable
	<<< Map.values
	<<< Map.mapMaybeWithKey \name requested ->
	Map.lookup name registry <#> \(RegistryVersion resolved) ->
	{ name, requested, resolved }

[f-f]

@JoelLefkowitz to answer to your excellent points above:

(Note: I'm happy to state my style preferences, but keep in mind that these are in fact preferences about how I like the codebases I contribute to. My main goal is to make code easy to read/understand/reason about, but someone else might have different preferences)

do you think it is more readable to use map and join instead of bind?

To me yes, map+bind immediately conveys the intent of operating on Arrays, I find bind sneakier in the sense that it depends on which monad we're using, which might not be apparent e.g. when operating on structures such as Effect (Array (Maybe a))

Would you also inline a large function that is only used once?

Yes, because functions that are used only once are generally situational so it makes sense to not expose them to the broader scope, or the rest of the code might be tempted to use them. If they are in the inner scope one has to think before moving the function around. We do this a lot in the Spago codebase, e.g. see here

From what I've seen type annotations don't usually appear in let blocks.

Type annotations in let blocks are as useful as type annotations at the top level, as they help the compiler to typecheck, and provide documentation to future readers. We do this a lot as well, e.g. see here

Are there any guidelines on when to follow this pattern and when it is preferable to write something like VersionResolution.resolutions over resolvePackageVersionsToRanges?

A lot of that comes down to how generic the function is. Things like length and print make sense if they are in a separate module, e.g. PackageName, or Range, etc.
Another thing to note in this case is that this function is not offering some static resolutions, but it is matching versions to ranges, which prompts some sort of verb in the name, e.g. resolve

[JoelLefkowitz]

Thanks @f-f I've constructed the warning message with the Loggable class now and I've taken some of the repetition out. For mapping the intermediate value from Map.mapMaybeWithKey I've added a bind + case expression rather than annotating the function as Partial.

Thank you for explaining your style preferences they're really insightful, especially regarding scoping functions. It resonates strongly with me to prioritise readability and I'd be quite interested to read the threads on discord that discuss these ideas.

[f-f]

This is great - thank you!