New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Manual page generation #2877

Merged
merged 6 commits into from Oct 22, 2015

Conversation

Projects
None yet
2 participants
@mmakowski
Copy link
Collaborator

mmakowski commented Oct 17, 2015

Manual page generation from command list defined in Main. Based on the approach and implementation in darcs.

Fixes #848.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Oct 17, 2015

Thanks! Looks good on a first glance.

import Data.Char (toUpper)
import Data.List (intercalate)

data CommandVisibility = Visible | Hidden

This comment has been minimized.

@23Skidoo

23Skidoo Oct 19, 2015

Member

Looks like this duplicates the functionality of CommandType in D.S.Command.

-- By hiding the type of flags for the UI allows construction of a list of all UIs at the
-- top level of the program. That list can then be used for generation of manual page
-- as well as for executing the selected command.
data CommandSpec action

This comment has been minimized.

@23Skidoo

23Skidoo Oct 19, 2015

Member

This looks like something that belongs better in D.S.Command. Consider refactoring that module.

data CommandSpec action
= forall flags. CommandSpec (CommandUI flags) (CommandUI flags -> Command action) CommandVisibility

commandFromSpec :: CommandSpec a -> Command a

This comment has been minimized.

@23Skidoo

23Skidoo Oct 19, 2015

Member

Ditto.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Oct 19, 2015

LGTM but I'd like to see some parts moved to D.S.Command.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Oct 19, 2015

A potential future improvement may be to add support for generating per-command manpages (e.g. cabal manpage install). Then we could make cabal help display a manpage when run from the terminal, a-la git help.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Oct 19, 2015

FYI, there are also some formatting problems (but I don't see them as impediment for merging):

formatting_problem

@mmakowski

This comment has been minimized.

Copy link
Collaborator

mmakowski commented Oct 19, 2015

@23Skidoo, thanks for the review. I will try to address the comments over the next couple of days.

Regarding cabal manpage install, I think that woud require making manpage a library-level special command similar to help. In the existing implementation I tried to confine the manpage concerns to cabal-install. It is still not clear to me which functionality belongs in the library and which in the command-line app, for example I find it surprising that help formatting is a library concern.

Another thing to consider with help displaying manual pages is that man is not availabe on Windows. IIRC git deals with it by making help open a browser pointing to online man page, I'm not sure if that is preferable to showing help in the console.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Oct 19, 2015

Regarding cabal manpage install, I think that woud require making manpage a library-level special command similar to help.

I am not so sure. If you can generate a manpage with information about all commands from cabal-install, it should also be possible to do the same for a single command (maybe after some refactoring of D.S.Command).

Code for help formatting and option handling lives in the Cabal lib because we want to support things like ./Setup foo --help. I think that manpage generation belongs in cabal-install.

Another thing to consider with help displaying manual pages is that man is not availabe on Windows.

In these cases we can just revert to the old mode of output.

@mmakowski

This comment has been minimized.

Copy link
Collaborator

mmakowski commented Oct 22, 2015

I have now moved command-related bits to D.S.Command and fixed formating of sandbox sub-commands.

@23Skidoo

This comment has been minimized.

Copy link

23Skidoo commented on 43be49e Oct 22, 2015

I'd prefer if you did a rebase and push -f instead of merging. Makes for cleaner history.

@23Skidoo

This comment has been minimized.

Copy link

23Skidoo commented on 9bc8c7a Oct 22, 2015

Alternatively you could have used void from Control.Monad. Just FYI, it's not a style requirement.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Oct 22, 2015

LGTM now.

23Skidoo added a commit that referenced this pull request Oct 22, 2015

@23Skidoo 23Skidoo merged commit 3923101 into haskell:master Oct 22, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Oct 22, 2015

Merged, thanks!

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