Added version information to haddock page title of executables #1336 #3959

holzensp · 2016-10-08T15:31:16Z

Minor request; add version numbers to the page titles of the haddock documentation of executables.

mention-bot · 2016-10-08T15:31:17Z

@holzensp, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dcoutts, @igfoo and @23Skidoo to be potential reviewers.

23Skidoo · 2016-10-08T15:37:04Z

Cabal/Distribution/Simple/Haddock.hs

-    }
+fromLibrary :: PackageDescription
+            -> Library
+            -> Verbosity


By convention, the Verbosity argument should go first.

Opted for this to not have to eta-expand things. I'll change.

23Skidoo · 2016-10-08T15:37:17Z

Cabal/Distribution/Simple/Haddock.hs

-fromExecutable :: Verbosity
+fromExecutable :: PackageDescription
+               -> Executable
+               -> Verbosity


23Skidoo · 2016-10-08T15:39:38Z

Cabal/Distribution/Simple/Haddock.hs

+type ModuleFiles = [(ModuleName.ModuleName, FilePath)]
+fromTarget :: (LocalBuildInfo -> ComponentLocalBuildInfo
+                -> target -> IO ModuleFiles)
+           -> (HaddockArgs -> HaddockArgs)


This argument seems unnecessary -- you can just do amendArgs <$> fromTarget ... in fromLibrary/fromExe.

Ouch... you're right. This is incremental hacking. Good catch

23Skidoo · 2016-10-08T15:40:45Z

Cabal/Distribution/Simple/Haddock.hs

-fromLibrary verbosity tmp lbi lib clbi htmlTemplate haddockVersion = do
-    inFiles <- map snd `fmap` getLibSourceFiles lbi lib clbi
+type ModuleFiles = [(ModuleName.ModuleName, FilePath)]
+fromTarget :: (LocalBuildInfo -> ComponentLocalBuildInfo


Bikeshedding: I'd rename it to fromLibOrExe.

In principle we could Haddock tests and executables, couldn't we? Given the type signature fromTarget seems appropriate.

I'm not passionate either way, but considering @ezyang's comment, I'll stick with fromTarget

23Skidoo · 2016-10-08T15:44:55Z

Looks OK. In the future, can you please split refactorings and functionality changes into different commits?

ezyang · 2016-10-08T18:33:17Z

Cabal/Distribution/Simple/Haddock.hs

-    inFiles <- map snd `fmap` getLibSourceFiles lbi lib clbi
+type ModuleFiles = [(ModuleName.ModuleName, FilePath)]
+fromTarget :: (LocalBuildInfo -> ComponentLocalBuildInfo
+                -> target -> IO ModuleFiles)


In general, if you have a higher-order argument like this, doc is especially important. In this case, the passed in function is supposed to list the sources of the module in question. But why isn't just inFiles passed in as an argument? (UPDATE: I guess the answer is, "because it saves less typing at the use site")

Yes, it was of typing at the use site... It's a subjective trade-off, but I went for compactness.

ezyang · 2016-10-08T18:38:02Z

Cabal/Distribution/Simple/Haddock.hs

-fromExecutable verbosity tmp lbi exe clbi htmlTemplate haddockVersion = do
-    inFiles <- map snd `fmap` getExeSourceFiles lbi exe clbi
-    ifaceArgs <- getInterfaces verbosity lbi clbi htmlTemplate
-    let vanillaOpts = (componentGhcOptions normal lbi bi clbi (buildDir lbi)) {


The rest of the comments are just stylistic fluff (and can be ignored), but this has got me worried. I don't see this code anywhere in the new diff. This code seems to be solving a very specific problem (interface file overwriting) and I don't see (1) any code in the new diff to handle it, or (2) a comment in the commit saying why this is not a problem anymore. It's possible it isn't (esp if the CI passes) but that is very non-obvious!

Line 295 in the new version (fromTarget); did I miss something?

You are right! This is what I get for assuming that the original code hadn't copy-pasted the damn code :)

holzensp

On splitting out into separate diffs (@23Skidoo); yes, you're right. Things had already happened when I realised. Apologies for the conflation.

holzensp · 2016-10-10T12:16:30Z

Cabal/Distribution/Simple/Haddock.hs

-    inFiles <- map snd `fmap` getLibSourceFiles lbi lib clbi
+type ModuleFiles = [(ModuleName.ModuleName, FilePath)]
+fromTarget :: (LocalBuildInfo -> ComponentLocalBuildInfo
+                -> target -> IO ModuleFiles)


Yes, it was of typing at the use site... It's a subjective trade-off, but I went for compactness.

holzensp · 2016-10-10T12:16:54Z

Cabal/Distribution/Simple/Haddock.hs

+type ModuleFiles = [(ModuleName.ModuleName, FilePath)]
+fromTarget :: (LocalBuildInfo -> ComponentLocalBuildInfo
+                -> target -> IO ModuleFiles)
+           -> (HaddockArgs -> HaddockArgs)


Ouch... you're right. This is incremental hacking. Good catch

holzensp · 2016-10-10T12:18:13Z

Cabal/Distribution/Simple/Haddock.hs

-    }
+fromLibrary :: PackageDescription
+            -> Library
+            -> Verbosity


Opted for this to not have to eta-expand things. I'll change.

holzensp · 2016-10-10T12:26:44Z

Cabal/Distribution/Simple/Haddock.hs

-fromLibrary verbosity tmp lbi lib clbi htmlTemplate haddockVersion = do
-    inFiles <- map snd `fmap` getLibSourceFiles lbi lib clbi
+type ModuleFiles = [(ModuleName.ModuleName, FilePath)]
+fromTarget :: (LocalBuildInfo -> ComponentLocalBuildInfo


I'm not passionate either way, but considering @ezyang's comment, I'll stick with fromTarget

holzensp · 2016-10-10T12:27:10Z

Cabal/Distribution/Simple/Haddock.hs

-fromExecutable :: Verbosity
+fromExecutable :: PackageDescription
+               -> Executable
+               -> Verbosity


holzensp · 2016-10-10T12:30:00Z

Cabal/Distribution/Simple/Haddock.hs

-fromExecutable verbosity tmp lbi exe clbi htmlTemplate haddockVersion = do
-    inFiles <- map snd `fmap` getExeSourceFiles lbi exe clbi
-    ifaceArgs <- getInterfaces verbosity lbi clbi htmlTemplate
-    let vanillaOpts = (componentGhcOptions normal lbi bi clbi (buildDir lbi)) {


Line 295 in the new version (fromTarget); did I miss something?

ezyang · 2016-10-15T22:09:36Z

I'm OK with merging this!

23Skidoo · 2016-10-15T23:57:17Z

IIUC, @holzensp wants to address some review comments before merging.

Mikolaj · 2021-05-26T19:25:11Z

@holzensp: hi! would you like to rebase/revisit this PR? I'm assuming the functionality is not implemented elsewhere, but if while rebasing you find that it is, this is also good to know and perhaps the current implementation may be improved using this old PR.

Kleidukos · 2023-05-17T06:27:56Z

Marking this PR as draft 🙂

Added version information to haddock page title of executables haskel…

54a1a34

…l#1336

23Skidoo reviewed Oct 8, 2016

View reviewed changes

23Skidoo mentioned this pull request Oct 8, 2016

You're awesome! We're giving you commit bits. #3567

Closed

ezyang reviewed Oct 8, 2016

View reviewed changes

23Skidoo added blocked: action-required type: enhancement labels Oct 9, 2016

holzensp commented Oct 10, 2016

View reviewed changes

ezyang removed the blocked: action-required label Oct 15, 2016

23Skidoo added the blocked: action-required label Oct 15, 2016

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 1, 2022

ulysses4ever removed the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 3, 2022

Kleidukos marked this pull request as draft May 17, 2023 06:28

ulysses4ever added the Cabal: cmd/haddock label Jun 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added version information to haddock page title of executables #1336 #3959

Added version information to haddock page title of executables #1336 #3959

holzensp commented Oct 8, 2016

mention-bot commented Oct 8, 2016

23Skidoo Oct 8, 2016

holzensp Oct 10, 2016

23Skidoo Oct 8, 2016

holzensp Oct 10, 2016

23Skidoo Oct 8, 2016

holzensp Oct 10, 2016

23Skidoo Oct 8, 2016

ezyang Oct 8, 2016

holzensp Oct 10, 2016

23Skidoo Oct 15, 2016

23Skidoo commented Oct 8, 2016

ezyang Oct 8, 2016 •

edited

holzensp Oct 10, 2016

ezyang Oct 8, 2016

holzensp Oct 10, 2016

ezyang Oct 10, 2016

holzensp left a comment

holzensp Oct 10, 2016

holzensp Oct 10, 2016

holzensp Oct 10, 2016

holzensp Oct 10, 2016

holzensp Oct 10, 2016

holzensp Oct 10, 2016

ezyang commented Oct 15, 2016

23Skidoo commented Oct 15, 2016

Mikolaj commented May 26, 2021

Kleidukos commented May 17, 2023

Added version information to haddock page title of executables #1336 #3959

Are you sure you want to change the base?

Added version information to haddock page title of executables #1336 #3959

Conversation

holzensp commented Oct 8, 2016

mention-bot commented Oct 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

23Skidoo commented Oct 8, 2016

ezyang Oct 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holzensp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezyang commented Oct 15, 2016

23Skidoo commented Oct 15, 2016

Mikolaj commented May 26, 2021

Kleidukos commented May 17, 2023

ezyang Oct 8, 2016 •

edited