haskell / haskell/cabal

Consider bringing back tag parsing to Text instance on Version

Open
#4,408 0 comments 0 reactions 0 assignees View on GitHub
type: refactor
Dominant language
Haskell
Stars
1.7k
Forks
750
Avg merge
4d 3h
Merged PRs (30d)
28

Description

I recently discovered a funny bug in Hackage:

```
pkgid <- case simpleParse pkgidStr of
Just pkgid
| (== nullVersion) . packageVersion $ pkgid
-> throwError $ "Invalid package id " ++ quote pkgidStr
++ ". It must include the package version number, and not just "
++ "the package name, e.g. 'foo-1.0'."

| display pkgid == pkgidStr -> return (pkgid :: PackageIdentifier)

| not . null . versionTags . packageVersion $ pkgid
-> throwError $ "Hackage no longer accepts packages with version tags: "
++ intercalate ", " (versionTags (packageVersion pkgid))

_ -> throwError $ "Invalid package id " ++ quote pkgidStr
++ ". The tarball must use the name of the package."
```

This uses the Text instance on PackageId to parse a package identifier, and then looks in the versionTags of the resulting version to see if there are tags or not.

Actually, this totally doesn't do the right thing at all, because the parser for version numbers DROPPED the tags.

```
instance Text Version where
disp (Version branch _tags) -- Death to version tags!!
= Disp.hcat (Disp.punctuate (Disp.char '.') (map Disp.int branch))

parse = do
branch <- Parse.sepBy1 parseNat (Parse.char '.')
-- allow but ignore tags:
_tags <- Parse.many (Parse.char '-' >> Parse.munch1 isAlphaNum)
return (Version branch [])
```

So there will NEVER be tags in the resulting `Version`, and the check is a no-op. That's not what you want!

Now that we have a distinct `Version` data type for "real" Cabal versions, we should make the Data.Version instance properly reflect the actual data type it represents, to prevent this sort of bug.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.