Change MungedPackageName to not be opaque #5812
Conversation
|
Travis is green. Review would be highly appreciated. |
i.e. strict pair of PackageName and LibraryName the legacy conversion is done via Pretty/Parsec instances. Change of `Maybe UnqualComponentName` to `LibraryName` caused a cascade of other changes, but they all seem to be good changes. In the sense, they made many comments not-so-necessary. Add Distribution.Types.PackageName.Magic for special package names. Updates in cabal-install are mostly trivial type error driven changes. I removed few (deprecated) `Text` instances: `MungedPackageId`, `MungedPackageName` and `LibraryName`. Turns out only a `Pretty` part was used, so it was easy to update. Note: `LibraryName` doesn't have `Pretty` / `Parsec` instances as it's either parsed/printed as a `ComponentName` or `UnqualComponentName`, never stand alone.
There was a problem hiding this comment.
lgtm except some comments
When this is merged we can also mark https://github.com/haskell/cabal/wiki/Potential-Refactors#maybe-unqualcomponentname as done. Thanks for this!
| @@ -370,6 +370,7 @@ library | |||
| Distribution.Types.LibraryName | |||
| Distribution.Types.MungedPackageName | |||
| Distribution.Types.PackageName | |||
| Distribution.Types.PackageName.Magic | |||
There was a problem hiding this comment.
Shouldn't this be a separate commit?
There was a problem hiding this comment.
Not easily. Won't do.
| @@ -1230,7 +1229,7 @@ selectDependency pkgid internalIndex installedIndex requiredDepsMap | |||
| -- even if there is a newer installed library "MyLibrary-0.2". | |||
| case Map.lookup dep_pkgname internalIndex of | |||
| Just cname -> if use_external_internal_deps | |||
| then do_external (Just cname) <$> Set.toList libs | |||
| then do_external (Just $ maybeToLibraryName cname) <$> Set.toList libs | |||
There was a problem hiding this comment.
I think the internalIndex should be Map PackageName LibraryName instead of Map PackageName (Maybe UnqualComponentName) (I checked where it originates and it seems to be doable)
There was a problem hiding this comment.
Won't fix. Properly done current Map PackageName (Maybe UnqualComponentName) into a newtype over Map PackageName LibraryName
| <$> optionalFieldDefAla "name" MQuoted basicName (mungedPackageName emptyInstalledPackageInfo) | ||
| <*> optionalFieldDefAla "version" MQuoted basicVersion nullVersion | ||
| <*> optionalField "package-name" basicPkgName | ||
| <*> optionalField "lib-name" basicLibName | ||
| where | ||
| mkBasic n v pn ln = Basic n v pn (maybe LMainLibName LSubLibName ln) |
There was a problem hiding this comment.
basicLibName should be of type LibraryName
There was a problem hiding this comment.
it's not exported. wont'fix. (Also LibraryName doesn't have Parsec instance on purpose, so I'd need to introduce newtype etc.)
There was a problem hiding this comment.
Maybe even using InstalledPackageIndex would make sense, even it's not "installed", but "package index" anyway.
See commit messages.
cabal-installis not done. Asking for the review now already. I thinkcabal-installpart should be quite straight-forward fixing of type errors