-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
WIP: Man page generation #3174
WIP: Man page generation #3174
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this up!
I will say that not everything has to be resolved in this PR. One route we can take (as I partially mention in the comments) is if we have this either behind a unstable-man
feature flag or in a separate crate. We can then create a stablization tracking issue like #2861 that points to issues for any work that gets deferred out of this. Feel free to propose deferring items to issues and we can come to an agreement about what is the minimum for merging and what is deferrable
clap_generate/src/man.rs
Outdated
write( | ||
buf, | ||
format_args!( | ||
".TH {} {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can understand. The issue of how much to read into precedence or not when starting a contribution. iirc roff was originally written with clap man page generation in mind. The rest of clap_generate doesn't have dependencies because nearly all of it is generating for arbitrary programming languages rather than a well defined markup. I can see has added shlex at some point if we need to be fixing quoting and string joining problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@epage I can switch to using roff
/man
, but I'm worried about the lack of maintenance for those crates and they are missing a fair bit of functionality last I looked, I see you're also active in the rust-cli
repositories so if you'd be willing to I can send contributions there and incorporate them back into this 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into reviving the roff crate.
Depending on how far along you are, I think i'm leaning towards let's get this crate merged and iterate on it from there. There will always be more improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't notice this comment when I started working on converting this to using my fork of roff-rs
, I've just pushed a full refactor for this to use it 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you create a PR against the roff repo so I can start reviewing those changes and get a release out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated CI on the roff repo. I did make some changes to make rustfmt / clippy happy. Feel free to overwrite those with your changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened a PR for my changes at rust-cli/roff-rs#14. I had also made some changes to the CI but yours is far more robust, so I've removed my changes.
clap_generate/src/man.rs
Outdated
} | ||
|
||
/// Add a custom section to the man pages. | ||
pub fn section(mut self, title: impl Into<String>, body: Vec<impl Into<String>>) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering in #3108 if we should have a similar API as this for App
which would allow sharing these between App
and man pages.
The question after that is what if someone doesn't want something shared.
clap_generate/src/man.rs
Outdated
} | ||
|
||
fn render_subcommands(&self, buf: &mut String) { | ||
let commands: Vec<_> = self.app.get_subcommands().collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to args, what order should we show subcommands?
I'll go over the comments in more detail tomorrow and refactor accordingly, but as you mention this is nearly more a POC than WIP for manpage generation. I hadn't thought about having a |
I can understand. The issue of how much to read into precedence or not when starting a contribution. iirc |
I've refactored and split it out into a |
clap_generate/src/man.rs
Outdated
if let Some(help) = opt.get_help() { | ||
buf.push_str(&indent(4, help)); | ||
} | ||
|
||
if let Some(help) = opt.get_long_help() { | ||
buf.push_str("\n.sp\n.sp\n"); | ||
buf.push_str(&indent(4, help)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we generate the help, we also include
- default values
- relevant env variable (granted, we shouldn't include the current env value)
- possible values
- A question for possible values is they can have help text. This is currently only being used by the completion scripts. Do assume that the
long_help
will include that information or should we detect when help for possible values is included and show the possible values as a list with the associated help text
- A question for possible values is they can have help text. This is currently only being used by the completion scripts. Do assume that the
Should we be including these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that including default values and environment variables are useful, not so sure about how including possible values would look though. I'll give it a go!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there are hidden settings for all of those, and some also have a global version (AppSettings::HidePossibleValues
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll give it a go!
Keep in mind I am trying to record everything as I think of it so we don't forget but not all of this has to be done in the first pass. We can get a subset and do incremental improvements from there, collecting feedback and maybe getting others chipping in since there is a lot of interest in this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation shows the default values in a [default: <values>]
block after the option and has a paragraph added to the bottom about the environment variable if it exists and is visible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about possible values, their help message, and the various hide settings for envs, defaults, values, etc.
Oh, and should visible aliases be shown?
clap_man/src/lib.rs
Outdated
/// 1. Executable programs or shell commands | ||
/// 2. System calls (functions provided by the kernel) | ||
/// 3. Library calls (functions within program libraries) | ||
/// 4. Special files (usually found in /dev) | ||
/// 5. File formats and conventions, e.g. /etc/passwd | ||
/// 6. Games | ||
/// 7. Miscellaneous (including macro packages and conventions), e.g. man(7), groff(7) | ||
/// 8. System administration commands (usually only for root) | ||
/// 9. Kernel routines [Non standard] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we provide either constants for this or even switch to an enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pulled this out to roff-rs
.
clap_man/src/render.rs
Outdated
} | ||
|
||
if app.has_subcommands() { | ||
write!(buf, "[{}]", subcommand_heading(app).to_lowercase()).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We also have a
App::subcommand_value_name
that is intended to go here.
I also don't think we should be outguessing the user on the casing
- There are some
AppSettings
for controlling whether the subcommand is required or not. I think they areSubcommandRequired
andSubcommandRequiredElseHelp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've implemented this, but I'm not 100% sure it's correct.
clap_man/src/render.rs
Outdated
} | ||
|
||
for pos in items.iter().filter(|a| a.is_positional()) { | ||
let required = pos.is_set(ArgSettings::Required); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An argument can also be required if its arg group is required
I think I've created all of the needed issues. Notice if I missed any? |
Probably how to handle subcommands (separate pages/in the main document), and that |
I've created an issue for subcommands and reached out to killercup. They are a past mainainer and still around, so that should be seemless. |
I was just about to publish I'm hesitant to change our naming style because it will be a common slip up for users to use the wrong one. Any ideas for an alternative name or are we stuck with this being inconsistent? |
|
All of the |
Ah, that's unfortunate, yeah. Maybe something like |
I pinged the crates.io team about this and Carol says renaming isn't supported, see rust-lang/crates.io#728. I'm leaning towards I can't think of anything that complements |
|
What about |
I'm fine with any name, but I'm mostly just a bystander at this point. |
My 2¢ is that "manual" is ambiguous and could suggest documentation for Clap as much as a man-page generator. Obviously that isn't how most Rust project documentation works, but "mangen" is a lot more suggestive of what actually goes on here. |
I just released clap_mangen-v0.1.0. @sondr3 I'll leave it to you if you want to do any social media announcements. |
I've been missing automatic man page generation for my CLI applications written using
clap
for a while, so I've decided to try/start to implement it on my own. This is currently a WIP, but it generates a very basic man page covering the arguments, positionals and subcommands with the most common sections that I've been able to find. I see that there has been a lot of movement on issues regarding help generation, so I may have started working on this at a bit of an unfortunate time 😄 This will close #552.The API should not be considered final, and is something I've cobbled together by looking at the shell completion generators and kinda worked it into an API for man pages. I'm creating this PR now to gather feedback on where to go from here and how it looks.
Todos
git
does)Output from `man` example