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

feat(cargo-shuttle): generate manpage #1388

Merged
merged 10 commits into from
Dec 6, 2023

Conversation

selectiveduplicate
Copy link
Contributor

@selectiveduplicate selectiveduplicate commented Nov 16, 2023

Add the ability to generate man page for cargo-shuttle.

Description of change

Summary

  • Adds the feature to generate man page for cargo-shuttle.
  • Uses separate generate sub-commands shell and manpage to generate shell completions and man page respectively (breaking API change).
  • No longer uses an environment variable for the output path of shell completions ((breaking API change).

Details

  1. Introduces a new sub-command manpage for the generate sub-command to generate man page with clap-mangen.

    To verify the changes, we can run the following from the project root shuttle:

    cargo run --package cargo-shuttle -- generate manpage

    This generates the man page in the current working directory by default. We can set a custom directory by setting the OUT_DIR environment variable:
    This generates the man page and renders it to the standard output. You can redirect the output to a .1 man page file and read it with man:

    cargo run --package cargo-shuttle -- generate manpage > cargo-shuttle.1
    man -l cargo-shuttle.1
  2. Brings both shell completions and man page generation under separate generate sub-commands. For shell completions, this means now you must generate shell completions with the following command:

    cargo run --package cargo-shuttle -- generate shell SHELL_NAME

    Instead of the previous API:

    cargo run --package cargo-shuttle -- generate --shell SHELL_NAME

    The output option for shell completion no longer relies on the value of the OUTPUT environment variable. If you want a custom path, use the --output option with generate shell:

    cargo run --package cargo-shuttle -- generate shell -h
    Generate shell completions
    
    Usage: cargo-shuttle generate shell [OPTIONS] <SHELL>
    
    Arguments:
      <SHELL>  The shell to generate shell completion for [possible values: bash, elvish, fish, powershell, zsh]
    
    Options:
      -o, --output <OUTPUT>
              Output to a file (stdout by default)
          --working-directory <WORKING_DIRECTORY>
              Specify the working directory [default: .] [aliases: wd]
          --name <NAME>
              Specify the name of the project (overrides crate name)
      -h, --help
              Print help

Closes #1377.

How has this been tested? (if applicable)

Please see the change description above.

Introduce a new option `--manpage` for the `generate` subcommand to generate manpage with clap-mangen
Copy link
Contributor

@orhun orhun left a comment

Choose a reason for hiding this comment

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

Looks good, just needs a bit of tweaking 🐻

cargo-shuttle/src/args.rs Show resolved Hide resolved
cargo-shuttle/src/args.rs Outdated Show resolved Hide resolved
cargo-shuttle/src/args.rs Outdated Show resolved Hide resolved
…ional

Make `--shell` optional and remove the `env` attribute; otherwise it interferes
with the `--manpage` option. Rendering to standard output is a better choice
just like we do for shell completions.
Comment on lines 137 to 144
#[arg(short, long)]
shell: Option<Shell>,
/// Output to a file (stdout by default)
#[arg(short, long, env)]
output: Option<PathBuf>,
/// Generate manpage to the standard output
#[arg(short, long)]
manpage: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Since they are mutually exclusive, it feels cleaner if they were separate subcommands: cargo shuttle generate shell <SHELL> and cargo shuttle generate manpage. Although a breaking change, could this be acceptable? @oddgrd

Copy link
Member

Choose a reason for hiding this comment

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

The OUTPUT env var usage feels weird as well. Could be a risk of collision with other programs using it. How do we feel about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah using OUTPUT is a bit risky. OTOH although the subcommand approach might look cleaner it would add unnecessary complexity to the code IMO. But I'm fine with either way - and I would say let's have this ready soon and not get caught up into discussion too much 🐻

Copy link
Contributor

Choose a reason for hiding this comment

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

Since they are mutually exclusive, it feels cleaner if they were separate subcommands: cargo shuttle generate shell <SHELL> and cargo shuttle generate manpage. Although a breaking change, could this be acceptable?

I think having them as separate subcommands makes the most sense. Regarding the output arg, we could remove the env. That makes two breaking changes, but I doubt it will affect many (if any) users.

@selectiveduplicate
Copy link
Contributor Author

I've added the changes and updated the PR description. Apologies for any mistakes.

Copy link
Member

@jonaro00 jonaro00 left a comment

Choose a reason for hiding this comment

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

It works, but the manpage only contains the global args and references other manpages for each subcommand (not very useful in our case), and I can find any way to generate proper manpages for subcommands or make the default manpage contain the manuals for all subcommands 🤔

@selectiveduplicate
Copy link
Contributor Author

It works, but the manpage only contains the global args and references other manpages for each subcommand (not very useful in our case), and I can find any way to generate proper manpages for subcommands or make the default manpage contain the manuals for all subcommands 🤔

Yeah you're right; it doesn't have much 😅 . I think to have manual for the sub-commands as well, we need to iterate over them using get_subcommands.

@selectiveduplicate
Copy link
Contributor Author

And since we're outputting to stdout, I wonder how we can do separate man pages for the sub-commands. As for dumping everything into one manual, maybe we can do this:

fn generate_manpage(&self) -> Result<CommandOutcome> {
    let app = Command::command();
    let output = std::io::stdout();
    let mut output_handle = output.lock();
    Man::new(app.clone()).render(&mut output_handle)?;
    for subcommand in app.get_subcommands().cloned() {
        Man::new(subcommand.clone()).render(&mut output_handle)?;
        if subcommand.has_subcommands() {
            for sb in subcommand.get_subcommands().cloned() {
                Man::new(sb).render(&mut output_handle)?;
            }
        }
    }

    Ok(CommandOutcome::Ok)
}

Just wanted to discuss the approach without bloating the commit history. 😅

@jonaro00
Copy link
Member

I quickly tested changing the input Command to Man to be a subcommand, but the result was an empty manpage, so not sure what was happening there.

@jonaro00
Copy link
Member

If you find a way to get everything into one manual, go for it.

@selectiveduplicate
Copy link
Contributor Author

We should now have everything in the man page.

@jonaro00
Copy link
Member

Cool. I see that a lot of parts are repeated (title and footer for each manpage), so we could customize which parts of the manpage are printed for each section to make it more unified.

@selectiveduplicate
Copy link
Contributor Author

Cool. I see that a lot of parts are repeated (title and footer for each manpage), so we could customize which parts of the manpage are printed for each section to make it more unified.

Ah yes. Pretty sure it's the title section of the man pages that's repeating. I don't think clap_mangen has an intutiive API for this; we may need to implement a builder pattern or something ourselves to customize the rendering in a nice way.

@jonaro00
Copy link
Member

They do have render_* methods for custom rendering: https://docs.rs/clap_mangen/latest/clap_mangen/struct.Man.html#method.render_title

@selectiveduplicate
Copy link
Contributor Author

We should have less repetitions now.

Copy link
Contributor

@orhun orhun left a comment

Choose a reason for hiding this comment

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

Tested and LGTM

@orhun orhun merged commit 9bf94e8 into shuttle-hq:main Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement]: Generate manpage with clap-mangen for cargo-shuttle
4 participants