Man gen #6
Man gen #6
Conversation
I've only briefly looked at this -- this might require you to port structopt to clap 3, right? Maybe we should change the milestone to target clap_derive instead? |
|
@killercup I'm okay with that, but it's worth noting that clap_derive is still in flux. Tested it a few weeks ago and couldn't get it to compile. Insgead we should perhaps just target clap3 for the initial demo? |
| } | ||
| ``` | ||
| Preview by running: |
There was a problem hiding this comment.
I hate that you don't put empty lines around code blocks. Just so you know. :P
There was a problem hiding this comment.
Thanks for sharing; your comment has been noted 😛
There was a problem hiding this comment.
As a side note: I'll happily switch to whichever style is conventional the moment rustfmt is able to do so automatically. Can't be bothered to otherwise :P
There was a problem hiding this comment.
brb, doing cargo new mdfmt
|
|
||
| use clap::{App, Arg, ArgSettings}; | ||
| pub use man::*; |
There was a problem hiding this comment.
I'd only expose some very carefully selected items.
There was a problem hiding this comment.
I think if we can get #9 to land, this will also be solved. For now it mostly enforces the same behavior we'd have as putting everything in lib/ (e.g. less typing until we clean things up, haha).
src/man/mod.rs
Outdated
| /// mycmd - brief description of the application | ||
| /// ``` | ||
| #[inline] | ||
| pub fn description(page: Roff, name: &str, desc: &Option<String>) -> Roff { |
There was a problem hiding this comment.
Why is this marked as inline? The compiler should be clever enough to inline this if wise.
And why is it public? It's only used in render, right?
There was a problem hiding this comment.
Same for all the fns below
There was a problem hiding this comment.
I was thinking of exposing them all as public, but I reckon you're right and we should just keep them private. Will change!
| description: Option<String>, | ||
| argument: String, | ||
| default: Option<String>, | ||
| ) -> Self { |
There was a problem hiding this comment.
That's quite a lot of params for one function.
Looking at this it might make sense to make the underlying Option type public (but not its fields) and expose a builder patter for it, too. (You can probably use a derive).
Same for consistency's sake for the other similar methods.
There was a problem hiding this comment.
I agree, thinking something along the lines of #2 (comment) would be better.
I kind of want to merge the API so we can start working on demos; and then quickly follow it up with a separate API to get the naming and things closer to what's outlined in the issue above. Definitely agree this should be better!
src/man/author.rs
Outdated
| @@ -0,0 +1,6 @@ | |||
| /// An author entry. | |||
| #[derive(Debug)] | |||
There was a problem hiding this comment.
You can probably also derive Clone. Not necessary right now but you're not using Debug, too
|
I think this should be good enough for now. - Going to merge so we can continue hacking on this ✨ |
Adds the man page generation code. Includes an example. Works towards: https://github.com/rust-lang-nursery/cli-wg/issues/42.
Review-wise, this PR is pretty early. I would like to ask to discuss any API design issues in #2, and focus the review on implementation issues only.
Once we land this the next step would be to wire it up to
structoptto hit the milestone's goal. Thanks!Example Output