Skip to content
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

Improvements to the manpage #903

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Improvements to the manpage #903

wants to merge 8 commits into from

Conversation

Julow
Copy link
Collaborator

@Julow Julow commented Oct 24, 2022

I updated the documentation for some commands and added consistency. Sub commands are grouped and sorted in sections, to show the useful commands first. I also updated some doc strings.

I added some documentation about dependency between compile and link phases.

The last commit concatenate the output of all --help into one page in the documentation. It's not easy to navigate because it's big and the TOC is not very readable. I'm considering generating a different page for each subcommand, what do you think?

src/odoc/bin/main.ml Outdated Show resolved Hide resolved
@@ -748,7 +778,8 @@ module Targets = struct

let cmd = Term.(const list_targets $ Compile.dst $ Compile.input)

let info = Term.info "compile-targets" ~doc:"TODO: Fill in."
let info =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is a little less obvious - it's quite possible odig is using this. I don't think the reference driver uses it, nor does/will dune, as dune's not really built for dynamic targets.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it does.

Not sure what's the dune comment is about, but in general it's good for build systems if they can ask the tools which paths they are going to read and write, rather than have build tools guess, discover or hardcode the file systems effects.

(A good example of this is support-files-targets, if all goes well odig will have nothing to do to support the new math support)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the point of this command as there's no reason for the compile command to output several files. Especially that it takes a -o.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there's value in operational genericity. If all tools worked that way for all their steps, build incrementalization logics would be fully generic.

This benefits both build system (no tool specific logic) and the tools themselves (they can change the way they want to store intermediate results on the file system without having to bother about existing build logics).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The output of compile must be given to link and the build system has to know that. I think this adds untested code paths and complexity rather than generality.

The build system must be changed every time Odoc changes so it has to stay simple to be able to adapt fast. Jon mentions Dune because it is built in a way that dynamic targets are hard to represent (a command that outputs an unknown number of files). It's trivial however to have a staticly known list of inputs and outputs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're talking about compile-targets here, not html-targets.

The discussion is running in circle. Goto.

Copy link
Collaborator Author

@Julow Julow Nov 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your comment doesn't explain why it's such a hassle to express that odoc compile -o foo will write foo but it's convenient to express that it will output the result of odoc compile-targets -o foo.

You have to describe somewhere that odoc compile-targets exists. Please implement it like that in your own DSL:

let compile_targets output = [ output ]

The discussion is running in circle because you refuse to answer my point. We are both talking in the void, it's your fault.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are both talking in the void, it's your fault.

Coming from you I will happily take all the blame.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep the discussion on the code not the people. We're not planning on removing the subcommand, so the discussion is purely about whether we document it in the legacy section or the support section. Since we have at least one user -- odig -- and there is no alternative other than assuming odoc's behaviour, let's put it in the support section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I moved the command to the support section.

src/odoc/bin/main.ml Outdated Show resolved Hide resolved
src/odoc/bin/main.ml Outdated Show resolved Hide resolved
@Julow
Copy link
Collaborator Author

Julow commented Nov 21, 2022

I've changed the manpage generator tool to parse odoc --help and extract more information: order of the commands, sections, summaries.

@jonludlam
Copy link
Member

Something is going wrong on macOS - CI is failing and I've seen a similar thing locally too.

@Julow
Copy link
Collaborator Author

Julow commented Nov 29, 2022

I've removed the manpage parsing from the runtest alias, so it cannot break the tests now. This can be improved after the release.

@jonludlam
Copy link
Member

I'd really really prefer not to be parsing the man output. I'd far rather work with the Cmdliner types and generate the mld file from them, but I think it's not possible today as the types are opaque. @dbuenzli do you have any suggestions? would you be receptive to either a new help output format -- 'mld' -- in cmdliner, or exposing something to allow this to be done more nicely?

@dbuenzli
Copy link
Contributor

would you be receptive to either a new help output format -- 'mld' -- in cmdliner, or exposing something to allow this to be done more nicely?

No. I'm a bit dubious about the usefulness of all this.

Man pages are a terrible documentation medium. Keep them at the minimum as the reference for the cli interface and have good high-level docs. I doubt there's much value in having them alongside the .mld files.

If you really have to then odoc --help=groff | groff -Thtml is your friend.

@Julow
Copy link
Collaborator Author

Julow commented Nov 30, 2022

We can pass the values around without going through cmdliner and without parsing but as @dbuenzli says, perhaps it's not so useful ?

We can merge the improvements to the documentation without this, before the release: #912

The sections allow to quickly find the important commands.
The "Legacy pipeline" section give less importances to the no longer
recommended commands.
Document the dependencies in the compile and link steps.
Fill the missing documentation.
Improve consistency and formatting.
Sections are assigned while defining the list to make sure they are
still in sync.
The output of every 'odoc cmd --help' is wrapped into code blocks and
concatenated into a page in the documentation.
Allows to build the documentation on <4.10, as the generating program
can't run on these versions.
Useful feedback when modifying the CLI.
Reduce the complexity of the Dune rules for building the doc. Might
allow the reference driver to build it, for example.
Parse the output of 'odoc --help' in a quick and dirty way. It's also
possible to parse the name of the section, which is not used yet.

'Unix.open_process_args_in' only works in the intended way on >=4.12.
Instead use 'Filename.quote_command', which requires 4.10.
Sections are used to group commands by generating doc sections. The TOC
looks more readable. The first section is also nicer with some
separations between the list items.
It randomly break on different targets. It should be replaced by
something that do not depend on parsing textual output before it can be
enabled again by default.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants