Make 'cabal haddock' define __HADDOCK_VERSION__ when preprocessing. #1238

Merged
merged 2 commits into from Oct 16, 2013

Conversation

Projects
None yet
5 participants
@23Skidoo
Member

23Skidoo commented Mar 15, 2013

This is useful e.g. for "writing documentation that links to module A without explicitly qualifying everything, where A is not directly imported." (see the discussion in #926)

Fixes #1237.

@tibbe

This comment has been minimized.

Show comment Hide comment
@tibbe

tibbe Mar 15, 2013

Owner

This seems sensible to me.

Owner

tibbe commented Mar 15, 2013

This seems sensible to me.

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Mar 15, 2013

Member

See my comment in #926. But even if we do do that, there is perhaps still an argument for the cpp approach. I'd like to see what other people think though first.

Note that the reason we removed the __HADDOCK__ define for haddock2 was that when we were making the transition from haddock 1 to 2, it was important not to use __HADDOCK__. With haddock 1 it was not able to parse everything and so people were using conditional compilation. If we'd kept using conditional compilation then we would miss out on all the new goodness like working with GADTs and other things haddock2 supports but haddock1 did not. In fact often times haddock 2 could not parse/typecheck the module in the __HADDOCK__ case because of the hacks people were using. So it was a simple matter of correctness.

So if possible I'd like to avoid having to show different content to haddock than to ghc, and to solve the issues in haddock rather than with cpp. But others may disagree.

Member

dcoutts commented Mar 15, 2013

See my comment in #926. But even if we do do that, there is perhaps still an argument for the cpp approach. I'd like to see what other people think though first.

Note that the reason we removed the __HADDOCK__ define for haddock2 was that when we were making the transition from haddock 1 to 2, it was important not to use __HADDOCK__. With haddock 1 it was not able to parse everything and so people were using conditional compilation. If we'd kept using conditional compilation then we would miss out on all the new goodness like working with GADTs and other things haddock2 supports but haddock1 did not. In fact often times haddock 2 could not parse/typecheck the module in the __HADDOCK__ case because of the hacks people were using. So it was a simple matter of correctness.

So if possible I'd like to avoid having to show different content to haddock than to ghc, and to solve the issues in haddock rather than with cpp. But others may disagree.

@feuerbach

This comment has been minimized.

Show comment Hide comment
@feuerbach

feuerbach Mar 15, 2013

Contributor

So if possible I'd like to avoid having to show different content to haddock than to ghc, and to solve the issues in haddock rather than with cpp.

That's ideal, yes. But fixing issues in haddock takes time, and it takes even longer for the fixed version to get installed on the hackage server. It's nice to be able to hack around it in the meantime.

And it's not that bad if all you have to do is add an import to get a hyperlinked function. Sometimes it just crashes, unfortunately.

Contributor

feuerbach commented Mar 15, 2013

So if possible I'd like to avoid having to show different content to haddock than to ghc, and to solve the issues in haddock rather than with cpp.

That's ideal, yes. But fixing issues in haddock takes time, and it takes even longer for the fixed version to get installed on the hackage server. It's nice to be able to hack around it in the meantime.

And it's not that bad if all you have to do is add an import to get a hyperlinked function. Sometimes it just crashes, unfortunately.

@23Skidoo

This comment has been minimized.

Show comment Hide comment
@23Skidoo

23Skidoo Mar 16, 2013

Member

Since we don't want to add a haddock-options field (which would allow the user to do things like haddock-options: --optghc="-D__HADDOCK__" - though this wouldn't work either currently because we don't use the built-in preprocessor for Haddock >= 2.0), I'm +1 on this.

Member

23Skidoo commented Mar 16, 2013

Since we don't want to add a haddock-options field (which would allow the user to do things like haddock-options: --optghc="-D__HADDOCK__" - though this wouldn't work either currently because we don't use the built-in preprocessor for Haddock >= 2.0), I'm +1 on this.

@HeinrichApfelmus

This comment has been minimized.

Show comment Hide comment
@HeinrichApfelmus

HeinrichApfelmus Apr 7, 2013

Issue #888 makes it impossible for packages that use QuasiQuotation to generate Haddock documentation. Without a CPP flag, I can't even write a workaround. Hence, I am in favor of a CPP flag.

Issue #888 makes it impossible for packages that use QuasiQuotation to generate Haddock documentation. Without a CPP flag, I can't even write a workaround. Hence, I am in favor of a CPP flag.

@23Skidoo

This comment has been minimized.

Show comment Hide comment
@23Skidoo

23Skidoo Aug 22, 2013

Member

@dcoutts Do we want to include this in 1.18?

Member

23Skidoo commented Aug 22, 2013

@dcoutts Do we want to include this in 1.18?

@23Skidoo

This comment has been minimized.

Show comment Hide comment
@23Skidoo

23Skidoo Aug 23, 2013

Member

@dcoutts
There is definitely a need for this feature. Currently people have to use workarounds, e.g.: https://github.com/basvandijk/lifted-base/blob/master/Setup.hs

Member

23Skidoo commented Aug 23, 2013

@dcoutts
There is definitely a need for this feature. Currently people have to use workarounds, e.g.: https://github.com/basvandijk/lifted-base/blob/master/Setup.hs

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Aug 26, 2013

Member

Hmm, I'll think about it.

Generally I don't like the idea that we workaround these things in other tools/packages, rather than just fixing haddock. In addition, these workarounds hang around for ages, e.g. once it is fixed in haddock then we still end up using the workaround and so the docs drift from reality.

I'm somewhat suspicious of using __HADDOCK__ because that's the same define that we used for haddock 1.x and it is required that we not define __HADDOCK__ for any older code that was written for the haddock 1.x era otherwise it doesn't work with haddock 2 (haddock 1 didn't need to compile the code so people's workarounds involved all kinds of hacks that just fail when haddock 2 did it properly). Perhaps __HADDOCK2__ or something like that?

Member

dcoutts commented Aug 26, 2013

Hmm, I'll think about it.

Generally I don't like the idea that we workaround these things in other tools/packages, rather than just fixing haddock. In addition, these workarounds hang around for ages, e.g. once it is fixed in haddock then we still end up using the workaround and so the docs drift from reality.

I'm somewhat suspicious of using __HADDOCK__ because that's the same define that we used for haddock 1.x and it is required that we not define __HADDOCK__ for any older code that was written for the haddock 1.x era otherwise it doesn't work with haddock 2 (haddock 1 didn't need to compile the code so people's workarounds involved all kinds of hacks that just fail when haddock 2 did it properly). Perhaps __HADDOCK2__ or something like that?

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Aug 26, 2013

Member

Or perhaps define __HADDOCK_VERSION__ to the version, like the existing __GLASGOW_HASKELL__ > 606.

That would also help with the issue of workarounds. If you knew that the issue (e.g. QQ #888) were fixed in the next haddock version you could do:

#if __HADDOCK_VERSION__ > 2112
...
#else

Probably should include the full 3-digit haddock version, since bugs can be fixed in minor versions. Note that __GLASGOW_HASKELL__ only included the 2-digit version.

Or we could do it the same way we do it for libraries

/* tool haddock-2.11.1 */
#define VERSION_haddock "2.11.1"
#define MIN_VERSION_haddock(major1,major2,minor) (\
  (major1) <  2 || \
  (major1) == 2 && (major2) <  11 || \
  (major1) == 2 && (major2) == 11 && (minor) <= 1)

If we do then we probably need to make it not clash with libs of the same name. TOOL_VERSION_* perhaps?

Member

dcoutts commented Aug 26, 2013

Or perhaps define __HADDOCK_VERSION__ to the version, like the existing __GLASGOW_HASKELL__ > 606.

That would also help with the issue of workarounds. If you knew that the issue (e.g. QQ #888) were fixed in the next haddock version you could do:

#if __HADDOCK_VERSION__ > 2112
...
#else

Probably should include the full 3-digit haddock version, since bugs can be fixed in minor versions. Note that __GLASGOW_HASKELL__ only included the 2-digit version.

Or we could do it the same way we do it for libraries

/* tool haddock-2.11.1 */
#define VERSION_haddock "2.11.1"
#define MIN_VERSION_haddock(major1,major2,minor) (\
  (major1) <  2 || \
  (major1) == 2 && (major2) <  11 || \
  (major1) == 2 && (major2) == 11 && (minor) <= 1)

If we do then we probably need to make it not clash with libs of the same name. TOOL_VERSION_* perhaps?

@23Skidoo

This comment has been minimized.

Show comment Hide comment
@23Skidoo

23Skidoo Aug 26, 2013

Member

Leaving this open to remind me to add __HADDOCK_VERSION__ or TOOL_VERSION.

Member

23Skidoo commented Aug 26, 2013

Leaving this open to remind me to add __HADDOCK_VERSION__ or TOOL_VERSION.

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Aug 26, 2013

Member

Actually __HADDOCK_VERSION__ and TOOL_VERSION are orthogonal. The tool version stuff is just nice generic infrastructure that'd be available at all times. The point of a __HADDOCK_VERSION__ define is to only be defined when compiling via haddock.

Member

dcoutts commented Aug 26, 2013

Actually __HADDOCK_VERSION__ and TOOL_VERSION are orthogonal. The tool version stuff is just nice generic infrastructure that'd be available at all times. The point of a __HADDOCK_VERSION__ define is to only be defined when compiling via haddock.

@23Skidoo

This comment has been minimized.

Show comment Hide comment
@23Skidoo

23Skidoo Aug 26, 2013

Member

@dcoutts OK, updated the patch to define __HADDOCK_VERSION__.

Member

23Skidoo commented Aug 26, 2013

@dcoutts OK, updated the patch to define __HADDOCK_VERSION__.

Make 'cabal haddock' define __HADDOCK_VERSION__ when preprocessing.
This is useful e.g. for "writing documentation that links to module A without
explicitly qualifying everything, where A is not directly imported." (see the
discussion in #926)

Fixes #1237.
@23Skidoo

This comment has been minimized.

Show comment Hide comment
@23Skidoo

23Skidoo Oct 16, 2013

Member

Looks like everybody agrees that __HADDOCK_VERSION__ is useful, so merging.

Member

23Skidoo commented Oct 16, 2013

Looks like everybody agrees that __HADDOCK_VERSION__ is useful, so merging.

23Skidoo added a commit that referenced this pull request Oct 16, 2013

Merge pull request #1238 from 23Skidoo/define-haddock
Make 'cabal haddock' define __HADDOCK_VERSION__ when preprocessing.

@23Skidoo 23Skidoo merged commit 1d177fd into haskell:master Oct 16, 2013

@23Skidoo 23Skidoo deleted the 23Skidoo:define-haddock branch Oct 16, 2013

@hesselink hesselink referenced this pull request in silkapp/json-schema Jul 8, 2014

Open

Haddock generation error with GHC 7.6.3 #8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment