--haddock-for-hackage works with older Haddock#5496
Conversation
We error-out when `--quickjump` is specified on a version of Haddock that doesn't support it. However, if `--quickjump` was implied by `--haddock-for-hackage`, we should just warn. Fixes haskell#5494.
Cabal/Distribution/Simple/Haddock.hs
Outdated
| alt = "The generated documentation won't have the QuickJump feature." | ||
| case haddockTarget of | ||
| ForDevelopment -> die' verbosity msg | ||
| ForHackage -> warn verbosity (msg ++ "\n" ++ alt) |
There was a problem hiding this comment.
You probably want "\n\n" or " ", since there's no visual break this way.
There was a problem hiding this comment.
I didn't want a visual break - just a new line. Should there be more of a visual break?
23Skidoo
left a comment
23Skidoo
left a comment
There was a problem hiding this comment.
It may make sense to change the type of haddockQuickJump to something like Flag (Maybe Bool) so that we could distinguish the haddock --for-hackage case from haddock --for-hackage --quickjump.
|
@harpocrates do you have time to try to implement @23Skidoo suggestion? i.e. so that
|
|
@hvr Done. Now, any mention of Also noticed that I'd forgotten to let through a |
| case haddockTarget of | ||
| ForDevelopment -> die' verbosity msg | ||
| ForHackage -> warn verbosity (msg ++ "\n" ++ alt) | ||
| if Flag True == quickJmpFlag |
There was a problem hiding this comment.
This is not quite what @hvr suggested IIUC. Since --for-hackage implies --quickjump, all of --for-hackage, --for-hackage --quickjump, and --quickjump will now fail hard when using an older haddock.
There was a problem hiding this comment.
It is though (I've even tried it out). quickJmpFlag records what the quickjump flag was prior to the --for-hackage implication. That way, only --for-hackage --quickjump, and --quickjump will fail hard when using an older haddock - --for-hackage by itself is just a warning.
There was a problem hiding this comment.
I can confirm that @harpocrates' PR works as advertised; I tried the combinations old/new haddock, and --haddock-for-hackage, --haddock-quickjump, --haddock-for-hackage --haddock-quickjump, and they matched my expectations.
There was a problem hiding this comment.
Great! Don't we still get a warning when neither --for-hackage nor --quickjump were specified, but haddock is old?
There was a problem hiding this comment.
@23Skidoo I don't see any warning w/ e.g. GHC 8.2.2 (i.e. old haddock) and a plain cabal new-haddock -w ghc-8.2.2 w/o any additional flags.
|
also cherry-picked into 2.4 via 76933f4 |
|
@harpocrates thanks! |
4 participants
We error-out when
--quickjumpis specified on a version of Haddock thatdoesn't support it. However, if
--quickjumpwas implied by--haddock-for-hackage, we should just warn.Fixes #5494.
Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!