@since includes package name #749

harpocrates · 2018-02-09T15:11:27Z

This changes the rendering of @since annotations to include the package that they originated from. This fixes #452 and #550. Here is a snippet of instances from the array package which are actually re-exported from base. It is now clear that 2.1 really is referring to the version of base and not array:

This is not yet ready to merge:

I noticed something funky when generating the GHC-bundled stm docs. I have no idea why, but the package name seems to include the version. Anyone know why?
It might be worth trying to leave off the package name when the @since annotation is in the current package - I haven't considered this, but it is mentioned in the linked issues

alexbiehl · 2018-02-09T17:56:37Z

/ cc @hvr

harpocrates · 2018-02-09T20:36:26Z

I figured out the right way to get the package name: modulePackageInfo.

I took the liberty to update the comments around modulePackageInfo, but it would be nice if someone else could check that. As I understand, the comment's dire warning / TODO was actually outdated by fdcd190.

When the name of a package cannot be determined (for instance when running Haddock on a standalone test case and without passing --package-name/--package-version), @since annotations don't get qualified with a package.

Consequently, I propose that we not leave off the package name when in the current package since that would be indistinguishable from the case where the package name could not be determined.

harpocrates · 2018-02-09T20:39:11Z

haddock-library/src/Documentation/Haddock/Types.hs

-newtype Meta = Meta { _version :: Maybe Version } deriving (Eq, Show)
+data Meta = Meta { _version :: Maybe Version
+                 , _package :: Maybe Package
+                 } deriving (Eq, Show)


@alexbiehl I take it this also requires waiting till the next major GHC version, right? Should I move this PR to ghc-head?

hvr · 2018-02-10T09:30:20Z

Consequently, I propose that we not leave off the package name when in the current package since that would be indistinguishable from the case where the package name could not be determined.

Can we make this controllable via a flag? We already have a precedent for this: a flag for controlling wether to qualify names,

  -q QUAL       --qual=QUAL                      qualification of names, one of 
                                                 'none' (default), 'full', 'local'
                                                 'relative' or 'aliased'

...may make sense here as well.

harpocrates · 2018-02-10T13:53:30Z

Can we make this controllable via a flag? We already have a precedent for this: a flag for controlling wether to qualify names,

If you have an idea for what to specify, sure. Note I already emit a warning if the package name couldn't be determined.

In any case, I would be against a flag that just hides this feature. People expect tools to "just do it" and they rarely go digging into options. Just yesterday, I found out about --qual and mentioned it to my coworkers at Galois. None were previously aware of its existence, despite some people actively wanting something like that. I'd much prefer if we could avoid hiding features behind flags.

hvr · 2018-02-10T21:58:14Z

@harpocrates the reason I'm asking is, that if I understand it right, the package name can always be determined accurately when running Haddock for Hackage. So in cabal haddock --for-hackage mode, we want the mode which omits package name for the local package. I generally don't like having the package name prepended for the local package, as this is the 99% case, and it would easily reduce the signal/noise ratio for the 1% which need to stand out more.

The flag would give us a way to control wether to always prepend the package name (unless it cannot be determined) or only when it can be determined, and the package name doesn't refer to the local package name. The latter would be the mode cabal haddock --for-hackage would invoke; while the former could be the default.

In other words, the flag isn't about hiding a feature, but rather about controlling it, so that we can accommodate different needs.

harpocrates · 2018-02-11T00:50:11Z

@hvr I understand now, I appreciate the explanation.

Stray thought: would it not be preferable for Haddock to have a hard failure when running with this special flag enabled? Anyways we expect cabal to do the right thing and pass the package name, so if something goes wrong, a loud failure could be nice... That way, you know for sure that a since annotation without a package name is not the result of a package name not being resolved.

I don't care much either way on this, so I'll implement whatever you prefer. 😄

Also, any naming preferences for such a flag?

alexbiehl · 2018-02-11T14:35:30Z

Alec, Herbert and I decided to make a major bump right after ghc-8.4.1/haddock-2.19.0 are released. So we will land #748 and #749 soon! One thing I want to do before merging is making the testsuite work properly again.

alexbiehl · 2018-02-09T18:04:20Z

haddock-api/src/Haddock/InterfaceFile.hs

@@ -486,8 +486,13 @@ instance Binary a => Binary (TableCell a) where
        return (TableCell i j c)

 instance Binary Meta where
-  put_ bh Meta { _version = v } = put_ bh v
-  get bh = (\v -> Meta { _version = v }) <$> get bh
+    put_ bh (Meta v p) = do


This changes the Binary instance. Please also update binaryInterfaceVersion:

haddock/haddock-api/src/Haddock/InterfaceFile.hs

Lines 84 to 86 in 76d0f9b

binaryInterfaceVersion :: Word16

#if (__GLASGOW_HASKELL__ >= 802) && (__GLASGOW_HASKELL__ < 804)

binaryInterfaceVersion = 31

alexbiehl · 2018-02-09T18:05:06Z

haddock-library/src/Documentation/Haddock/Types.hs

@@ -33,7 +33,9 @@ import Data.Bitraversable
 -- meta-data to comments. We make a structure for this ahead of time
 -- so we don't have to gut half the core each time we want to add such
 -- info.
-newtype Meta = Meta { _version :: Maybe Version } deriving (Eq, Show)
+data Meta = Meta { _version :: Maybe Version
+                 , _package :: Maybe Package


Unfortunately this is again a breaking change.

hvr · 2018-02-12T08:23:11Z

@harpocrates well, an obvious name could be --since-qual (we only need the long-form imho) :-)

harpocrates · 2018-02-12T14:48:29Z

@hvr This is now done. The --since-qual expects either always (the default) or external. The external variant is the one that cabal haddock --for-hackage should invoke.

alexbiehl · 2018-02-12T14:58:12Z

Good, so we need a Cabal change as well here?

hvr · 2018-02-12T18:45:54Z

@alexbiehl yeah, but that one's imho low-prio, and we can if needed have the doc builder use a locally patched version if needed

harpocrates · 2018-02-12T23:02:42Z

@hvr I don't mind making the Cabal change once this gets merged. That's the next big thing I'd like to start hacking on.

alexbiehl · 2018-03-02T15:01:50Z

(Alec, I rebased your patches against ghc-8.4 branch which should have a working testsuite by now)

harpocrates · 2018-03-04T18:40:48Z

Thanks! I'll fix up the tests in the next couple of days.

harpocrates · 2018-03-08T06:23:30Z

I may end up waiting for rc2 before making tests pass. Right now, I'm running into this issue on my dev machine when running the HTML tests.

harpocrates · 2018-03-21T19:36:09Z

@alexbiehl I'm running into some fundamental problems with the testsuite (beyond having to run everything in an Ubuntu VM).

As I understand it, when html-test runs Haddock, it passes in a bunch of interface files (for base, array, process, etc.) from the GHC being used. However, since my changes tweak Meta in (and its Binary instance), I've had to bump binaryInterfaceVersion, which means that html-test cannot load in the existing interface files.

Should I re-open this against ghc-head?

I feel like the "right" thing to do here would be to re-generate the interface files every time we run tests which rely on them, yet that would increase a fair bit the time it takes the tests to run...

@SInCE

This means that '@SInCE' annotations can be package aware.

This should extract the package name for `@since` annotations the right way. I had to move `modulePackageInfo` around to do this and, in the process, I took the liberty to update it. Since it appears that finding the package name is something that can fail, I added a warning for this case.

alexbiehl · 2018-03-21T19:53:23Z

Oh, right. There will be a ghc-8.4.2 release soon. I am sure we want to include the newly themed haddock in there to spread adoption at least for the core packages, right? Let's put this in then too. Let's wait with the merge until then. Alec Theriault <notifications@github.com> schrieb am Mi., 21. März 2018, 20:36:

…

@alexbiehl <https://github.com/alexbiehl> I'm running into some fundamental problems with the testsuite (beyond having to run everything in an Ubuntu VM). As I understand it, when html-test runs Haddock, it passes in a bunch of interface files (for base, array, process, etc.) from the GHC being used. However, since my changes tweak Meta in (and its Binary instance), I've had to bump binaryInterfaceVersion, which means that html-test cannot load in the existing interface files. Should I re-open this against ghc-head? I feel like the "right" thing to do here would be to re-generate the interface files every time we run tests which rely on them, yet that would increase a fair bit the time it takes the tests to run... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#749 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AByiid9eJOzzPP0fLdxmCJjYZErK5hThks5tgquugaJpZM4SAD30> .

alexbiehl · 2018-03-21T19:55:09Z

I am not sure we even can regenerate for base interface files without ghcs build system. Alex Biehl <alex.biehl@gmail.com> schrieb am Mi., 21. März 2018, 20:53:

…

Oh, right. There will be a ghc-8.4.2 release soon. I am sure we want to include the newly themed haddock in there to spread adoption at least for the core packages, right? Let's put this in then too. Let's wait with the merge until then. Alec Theriault ***@***.***> schrieb am Mi., 21. März 2018, 20:36: > @alexbiehl <https://github.com/alexbiehl> I'm running into some > fundamental problems with the testsuite (beyond having to run everything in > an Ubuntu VM). > > As I understand it, when html-test runs Haddock, it passes in a bunch of > interface files (for base, array, process, etc.) from the GHC being > used. However, since my changes tweak Meta in (and its Binary instance), > I've had to bump binaryInterfaceVersion, which means that html-test > cannot load in the existing interface files. > > Should I re-open this against ghc-head? > > I feel like the "right" thing to do here would be to re-generate the > interface files every time we run tests which rely on them, yet that would > increase a fair bit the time it takes the tests to run... > > — > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > <#749 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AByiid9eJOzzPP0fLdxmCJjYZErK5hThks5tgquugaJpZM4SAD30> > . >

As discussed, this is still the usual case (and we should avoid being noisy for it). Although this commit is large, it is basically only about threading a 'Maybe Package' from 'Haddock.render' all the way to 'Haddock.Backends.Xhtml.DocMarkup.renderMeta'.

harpocrates · 2018-03-22T07:46:27Z

Sounds all good to me!

alexbiehl · 2018-03-27T04:49:43Z

Alec, it is getting hotter in terms of release approaching. Could you rebase your change one last time?

This controls when to qualify since annotations with the package they come from. The default is always, but I've left an 'external' variant where only those annotations coming from outside of the current package are qualified.

@SInCE

The @SInCE stuff needs only the package name passed in, so it makes sense to not be forced to pass in a version too.

harpocrates · 2018-03-27T05:40:28Z

Rebased. As soon as this gets merged, I'll update the Cabal side of things.

alexbiehl · 2018-03-27T06:35:17Z

For the record: I checked test output and only expected errors happened:

Links are broken because of incompatible binary interface version
Since output has changed

@SInCE

* Metadoc stores a package name This means that '@SInCE' annotations can be package aware. * Get the package name the right way This should extract the package name for `@since` annotations the right way. I had to move `modulePackageInfo` around to do this and, in the process, I took the liberty to update it. Since it appears that finding the package name is something that can fail, I added a warning for this case. * Silence warnings * Hide package for local 'since' annotations As discussed, this is still the usual case (and we should avoid being noisy for it). Although this commit is large, it is basically only about threading a 'Maybe Package' from 'Haddock.render' all the way to 'Haddock.Backends.Xhtml.DocMarkup.renderMeta'. * Bump binary interface version * Add a '--since-qual' option This controls when to qualify since annotations with the package they come from. The default is always, but I've left an 'external' variant where only those annotations coming from outside of the current package are qualified. * Make ParserSpec work * Make Fixtures work * Use package name even if package version is not available The @SInCE stuff needs only the package name passed in, so it makes sense to not be forced to pass in a version too.

@SInCE

* Metadoc stores a package name This means that '@SInCE' annotations can be package aware. * Get the package name the right way This should extract the package name for `@since` annotations the right way. I had to move `modulePackageInfo` around to do this and, in the process, I took the liberty to update it. Since it appears that finding the package name is something that can fail, I added a warning for this case. * Silence warnings * Hide package for local 'since' annotations As discussed, this is still the usual case (and we should avoid being noisy for it). Although this commit is large, it is basically only about threading a 'Maybe Package' from 'Haddock.render' all the way to 'Haddock.Backends.Xhtml.DocMarkup.renderMeta'. * Bump binary interface version * Add a '--since-qual' option This controls when to qualify since annotations with the package they come from. The default is always, but I've left an 'external' variant where only those annotations coming from outside of the current package are qualified. * Make ParserSpec work * Make Fixtures work * Use package name even if package version is not available The @SInCE stuff needs only the package name passed in, so it makes sense to not be forced to pass in a version too. (cherry picked from commit 978dbc8)

harpocrates commented Feb 9, 2018

View reviewed changes

alexbiehl reviewed Feb 11, 2018

View reviewed changes

alexbiehl force-pushed the feature/pkg-aware-since branch from e2e73aa to 94accd2 Compare March 2, 2018 14:59

alexbiehl force-pushed the feature/pkg-aware-since branch from 94accd2 to 6c2b52f Compare March 2, 2018 17:29

harpocrates mentioned this pull request Mar 21, 2018

Show where instances are defined #748

Merged

harpocrates added 3 commits March 21, 2018 12:50

Metadoc stores a package name

5eaa94f

This means that '@SInCE' annotations can be package aware.

Silence warnings

414912e

harpocrates added 2 commits March 21, 2018 12:55

Bump binary interface version

366fb8e

harpocrates force-pushed the feature/pkg-aware-since branch from 2670ae1 to f5b33d4 Compare March 22, 2018 07:45

harpocrates and others added 4 commits March 26, 2018 22:36

Add a '--since-qual' option

45b47ea

This controls when to qualify since annotations with the package they come from. The default is always, but I've left an 'external' variant where only those annotations coming from outside of the current package are qualified.

Make ParserSpec work

355ae37

Make Fixtures work

4bfd611

Use package name even if package version is not available

69e74ea

The @SInCE stuff needs only the package name passed in, so it makes sense to not be forced to pass in a version too.

harpocrates force-pushed the feature/pkg-aware-since branch from f5b33d4 to 69e74ea Compare March 27, 2018 05:39

alexbiehl merged commit 978dbc8 into haskell:ghc-8.4 Mar 27, 2018

alexbiehl mentioned this pull request Mar 27, 2018

Feature request: when re-exporting from another package, @since should be rendered differently #550

Closed

harpocrates deleted the feature/pkg-aware-since branch March 27, 2018 07:02

harpocrates mentioned this pull request Mar 27, 2018

Haddock deprecate hscolour, tweak options haskell/cabal#5236

Merged

4 tasks

harpocrates mentioned this pull request Apr 16, 2018

@since annotations should include the package name #800

Closed

	binaryInterfaceVersion :: Word16
	#if (__GLASGOW_HASKELL__ >= 802) && (__GLASGOW_HASKELL__ < 804)
	binaryInterfaceVersion = 31

@since includes package name #749

@since includes package name #749

Uh oh!

Conversation

harpocrates commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexbiehl commented Feb 9, 2018

Uh oh!

harpocrates commented Feb 9, 2018

Uh oh!

harpocrates Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

hvr commented Feb 10, 2018

Uh oh!

harpocrates commented Feb 10, 2018

Uh oh!

hvr commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harpocrates commented Feb 11, 2018

Uh oh!

alexbiehl commented Feb 11, 2018

Uh oh!

alexbiehl Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

harpocrates Feb 11, 2018

Choose a reason for hiding this comment

Uh oh!

alexbiehl Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

hvr commented Feb 12, 2018

Uh oh!

harpocrates commented Feb 12, 2018

Uh oh!

alexbiehl commented Feb 12, 2018

Uh oh!

hvr commented Feb 12, 2018

Uh oh!

harpocrates commented Feb 12, 2018

Uh oh!

alexbiehl commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harpocrates commented Mar 4, 2018

Uh oh!

harpocrates commented Mar 8, 2018

Uh oh!

harpocrates commented Mar 21, 2018

Uh oh!

alexbiehl commented Mar 21, 2018 via email

Uh oh!

alexbiehl commented Mar 21, 2018 via email

Uh oh!

harpocrates commented Mar 22, 2018

Uh oh!

alexbiehl commented Mar 27, 2018

Uh oh!

harpocrates commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexbiehl commented Mar 27, 2018

Uh oh!

Uh oh!

harpocrates commented Feb 9, 2018 •

edited

Loading

hvr commented Feb 10, 2018 •

edited

Loading

alexbiehl commented Mar 2, 2018 •

edited

Loading

harpocrates commented Mar 27, 2018 •

edited

Loading