Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

add verbose argument acceptance to commands #1110

Merged
merged 5 commits into from
May 19, 2020

Conversation

bradyjoslin
Copy link
Contributor

addresses #975

Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper left a comment

Choose a reason for hiding this comment

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

This seems fairly straightforward, though I'm wondering if we want to add a --verbose flag to different subcommands if they don't yet do anything.

At the very least this PR should also parse and send along verbose as a boolean to each subcommand.

Tagging @cloudflare/workers-devexp for comments

@EverlastingBugstopper EverlastingBugstopper requested a review from a team March 2, 2020 19:46
@exvuma
Copy link
Contributor

exvuma commented Mar 2, 2020

I agree with @EverlastingBugstopper it's confusing to have a verbose flag added to a subcommand if it doesn't do anything yet. But it shouldn't yell at you if you do add it, just shouldn't show up on the help if it does nothing.

Is it possible for the subcommands that don't do anything to just accept the verbose arg, but not show up with --help?

@EverlastingBugstopper
Copy link
Contributor

hmm.. i don't think that is possible, but on second thought the goal here is to prevent the commands from failing and this achieves that.

going to cc in @ashleymichal since she filed that issue and leave her to approve if she thinks it's good 🚀

@ashleymichal
Copy link
Contributor

i think it is more disruptive to complain when passed the verbose flag than to see it in the help and not see any difference in output. that being said, upon searching the clap docs i did find a couple of argsettings that might be helpful: Global and Hidden: https://docs.rs/clap/2.33.0/clap/enum.ArgSettings.html#variant.Global
https://docs.rs/clap/2.33.0/clap/enum.ArgSettings.html#variant.Hidden

@bradyjoslin
Copy link
Contributor Author

Great feedback @ashleymichal

A straightforward way of conditionally displaying the help text while always handling the verbose flag is to define two variables for the verbose argument, one with the hidden argument setting set to true. Example implementation below, where verbose_arg would be added to any subcommands that handles the verbose flag, and the silent_verbose_arg applied where it isn't.

    let verbose_arg = Arg::with_name("verbose")
        .long("verbose")
        .takes_value(false)
        .help("toggle verbose output");

    let silent_verbose_arg = verbose_arg.clone().hidden(true);
...
        .subcommand(
            SubCommand::with_name("init")
                .about(&*format!(
                    "{} Create a wrangler.toml for an existing project",
                    emoji::INBOX
                ))
                .arg(silent_verbose_arg.clone())
...
        .subcommand(
            SubCommand::with_name("preview")
                .about(&*format!(
                    "{} Preview your code temporarily on cloudflareworkers.com",
                    emoji::MICROSCOPE
                ))
                .arg(verbose_arg.clone())
...

As an alternative solution, I evaluated setting a global arg for verbose with the hidden flag enabled, but struggled finding a way to override the hidden argument setting for those subcommands that handle the flag without getting a runtime error for non-unique argument names.

If the two variable option is sufficient, happy to make the change.

@EverlastingBugstopper
Copy link
Contributor

Hey @bradyjoslin the two variable approach seems perfect

@ashleymichal
Copy link
Contributor

hey @bradyjoslin , would you like to take a stab at the two-variable approach? or should we pick this PR up?

@bradyjoslin
Copy link
Contributor Author

@ashleymichal - Hey, thanks for the reminder on this. Should be something I can tackle, I'll review where I left off in the next few days.

Hope you all are doing well - the recent additions to Wrangler have been awesome!

@bradyjoslin
Copy link
Contributor Author

Two-variable verbose handling changes done. Let me know if you see anything that needs refinement.

@ashleymichal ashleymichal merged commit 5079463 into cloudflare:master May 19, 2020
@ashleymichal ashleymichal added this to the 1.9.1 milestone May 19, 2020
@EverlastingBugstopper
Copy link
Contributor

Thanks @bradyjoslin!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants