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

Subcommands should default to inheriting the top-level author value #172

Closed
ssokolow opened this issue Mar 18, 2019 · 14 comments
Closed

Subcommands should default to inheriting the top-level author value #172

ssokolow opened this issue Mar 18, 2019 · 14 comments
Labels
enhancement We would love to have this feature! Feel free to supply a PR

Comments

@ssokolow
Copy link

ssokolow commented Mar 18, 2019

Currently, I have two issues with the default behaviour regarding the author attribute:

  1. It messed up help2man output
  2. As it runs counter to established convention in the rest of the Linux/UNIX command ecosystem, it makes me look egotistical to anyone who isn't familiar with the quirks of this specific argument-parsing convenience crate. (ie. That it's an opt-out thing, not an opt-in thing.)

That wouldn't be a huge issue normally, since it's as simple as adding author="" to the structopt attribute and I've put that in my project boilerplate, but subcommands don't inherit it.

That means that, if I don't want Stephan Sokolow <http, //www.ssokolow.com/ContactMe> cluttering up all the subcommands in my project, something that should look like this...

#[derive(StructOpt, Debug)]
#[structopt(author="", rename_all = "kebab-case")]
pub enum Command {
    /// Rip an audio CD
    Audio,

    /// Rip a PC CD-ROM
    CD,

    /// Rip a PC DVD-ROM
    DVD,

    /// Rip a Sony PlayStation (PSX) disc in a PCSX/mednafen-compatible file
    PSX,

    /// Rip a Sony PlayStation 2 disc into a PCSX2-compatible format
    PS2,

    /// Rip a cartridge connected to the PC via a Retrode
    Retrode,

...winds up looking like this:

#[derive(StructOpt, Debug)]
#[structopt(author="", rename_all = "kebab-case")]
pub enum Command {
    ///
    /// Rip an audio CD
    #[structopt(author="")]
    Audio,

    ///
    /// Rip a PC CD-ROM
    #[structopt(author="")]
    CD,

    ///
    /// Rip a PC DVD-ROM
    #[structopt(author="")]
    DVD,

    ///
    /// Rip a Sony PlayStation (PSX) disc in a PCSX/mednafen-compatible format
    #[structopt(author="")]
    PSX,

    ///
    /// Rip a Sony PlayStation 2 disc into a PCSX2-compatible format
    #[structopt(author="")]
    PS2,

    ///
    /// Rip a cartridge connected to the PC via a Retrode
    #[structopt(author="")]
    Retrode,

...which is both a ludicrous status quo (How often do you find a project where different authors are responsible for different subcommands within the same binary?) and harmful for code readability.

StructOpt should just use the top-level author value for all subcommands unless they explicitly specify otherwise.

(The blank \\\ at the beginning is a workaround for another help2man incompatibility which I'll be reporting separately once I have a moment to test whether it's StructOpt's fault or Clap's.)

EDIT: And I just discovered that my \n workaround messes up the subcommand listing in --help. I just can't win either way.

@TeXitoi TeXitoi added the enhancement We would love to have this feature! Feel free to supply a PR label Mar 18, 2019
@TeXitoi
Copy link
Owner

TeXitoi commented Mar 18, 2019

A PR forwarding the author to the subcommand will be accepted. Technically that's a breaking change, but it also look a bit like a bug, thus I'm not sure a major release is needed.

@ssokolow
Copy link
Author

ssokolow commented Mar 19, 2019

To be honest, I'm not sure I can make time to familiarize myself with the StructOpt codebase to the degree necessary to write such a PR.

How much complexity would you say is involved?

...not to mention that I just realized that a fix is significantly less useful to me than I thought, because I also need to work around a couple of other issues that are Clap's fault (clap-rs/clap#1432, clap-rs/clap#1184), and an "AppSettings::ColoredHelp not inherited buy subcommands" that I haven't confirmed is a Clap bug yet.

The most concise way to work around all the problems I've identified would just be to support inheriting the #[structopt(raw(template="HELP_TEMPLATE", setting = "structopt::clap::AppSettings::ColoredHelp"))] that I've wound up sticking on every subcommand.

(The custom template solves the author problem by omitting {author}.)

@TeXitoi
Copy link
Owner

TeXitoi commented Mar 19, 2019

I dont really know, if no refactoring is needed, quie simple, but it may need some refactoring.

Thé issue express a need with a solution. If anyone is interested, he can implement it. If you want to work on it, express it on the issue. You don't need to do it.

@CreepySkeleton
Copy link
Collaborator

Isn't is solved by #229?

@ssokolow
Copy link
Author

ssokolow commented Aug 24, 2019

Changing the default solves it for my author-related use case by making my use case the default, but changing the default without fixing inheritance behaviour is still likely to be surprising and counter-intuitive for anyone who wants an author value and it certainly makes it more complicated to restore the old behaviour.

I'll try to test the new version and get back to you on it more definitively within the next little while. Looking back at my old comments, it looks like there's more to it than just that.

(eg. The "AppSettings::ColoredHelp not inherited buy subcommands" issue which I forgot to narrow down to StructOpt vs. clap, and clap-rs/clap#1432)

@CreepySkeleton
Copy link
Collaborator

CreepySkeleton commented Aug 24, 2019

@ssokolow About AppSettings::ColoredHelp - it's pretty much intentional - structopt tries to mimic clap's behavior save for some very specific things like version. clap does not propagate color settings by default - so structopt does not either. You can work this around by using global_settings

@TeXitoi
Copy link
Owner

TeXitoi commented Aug 25, 2019

As we are now in sync with clap philosophy on this issue, I propose to close it if there is no objection.

@ssokolow
Copy link
Author

ssokolow commented Aug 28, 2019

Yeah, that works for ColoredHelp.

The last week has been a mess for me and I can never remember the syntax for pulling a dependency from a non-crates.io source.

Mind pasting what I need to toss in my Cargo.toml to verify whether #229 plus global_settings resolves everything I can think about for this?

(I suspect that I'll wind up still having to duplicate template = for every subcommand, due to clap-rs/clap#1432)

@TeXitoi
Copy link
Owner

TeXitoi commented Aug 28, 2019

structopt = { git = "https://..." }

On mobile, so by memory and not tested.

@CreepySkeleton
Copy link
Collaborator

cc @ssokolow , any updates?

@ssokolow
Copy link
Author

Sorry about that. Got blindsided by a nasty flu and I'm just starting to crawl out of that pit. I'll try to find time for this in the next few days.

@ssokolow
Copy link
Author

OK. Here are my results.

With the current HEAD (e0f74b0):

  1. I still need to add template(HELP_TEMPLATE) to every subcommand as a workaround for Default template is incompatible with help2man clap-rs/clap#1432. (is there something I missed that's analogous to global_setting but for template?)
  2. With the default template, this does appear to resolve my issue with the author attribute, leaving the missing blank line between the command and its description in clap's default template (and the resulting incompatibility with help2man as the only reason I still need to use a custom template.

@CreepySkeleton
Copy link
Collaborator

@ssokolow

  1. Well, this is not structopt's area of responsibility, we can't do much about it. It must be done in clap.

@TeXitoi we can close this

@ssokolow
Copy link
Author

  1. Understood.

  2. That's OK. I'll close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement We would love to have this feature! Feel free to supply a PR
Projects
None yet
Development

No branches or pull requests

3 participants