Implement default subcommand. #129

qhua948 · 2022-06-13T06:57:01Z

argh/src/lib.rs

miguelfrde · 2022-06-13T18:00:28Z

For reference, we need this in https://fuchsia-review.googlesource.com/c/fuchsia/+/688982

We would like to support the following:

ffx log watch
ffx log list
ffx log dump
ffx log [watch args]

So when calling ffx log it defaults to ffx log watch when the command isn't specified.

argh/src/lib.rs

argh/tests/lib.rs

README.md

argh/src/lib.rs

miguelfrde · 2022-06-14T21:03:58Z

argh/tests/lib.rs

+Usage: cmdname one --x <x>
+
+First subcommand.


Is it possible to not print these two lines? The goal is to instruct the user of cmdname that they can use the commands one and two but that by default the command one would be called and immediately surface the options of that command. I think these two lines make this a bit confusing. Could we instead do something like:

Top-level command. Options: --help display usage information Commands: one First subcommand. two Second subcommand. When no command is given, the commad `one` is the default. Allowing the additional: Options: --x how many x --help display usage information

This is a bit harder to achieve given the state of the code, I can add the line of

When no command is given, the commad one is the default. Allowing the additional

And do this in a later patch if that is okay with you

I did implement this in the end but it took a lot more code than what I'd like to do it.

argh_derive/src/lib.rs

erickt

While I'm not opposed to this functionality, could you describe more the need for this? Argh currently doesn't require a subcommand to be specified. You can do something like this right now:

use argh::FromArgs;

/// Top Level
#[derive(FromArgs, PartialEq, Debug)]
struct TopLevel {
    #[argh(subcommand)]
    nested: Option<MySubCommandEnum>,
}

#[derive(FromArgs, PartialEq, Debug)]
#[argh(subcommand)]
enum MySubCommandEnum {
    One(SubCommandOne),
    Two(SubCommandTwo),
}

#[derive(FromArgs, PartialEq, Debug)]
/// First subcommand.
#[argh(subcommand, name = "one")]
struct SubCommandOne {
    #[argh(option)]
    /// how many x
    x: usize,
}

#[derive(FromArgs, PartialEq, Debug)]
/// Second subcommand.
#[argh(subcommand, name = "two")]
struct SubCommandTwo {
    #[argh(switch)]
    /// whether to fooey
    fooey: bool,
}

fn main() {
    let a: TopLevel = argh::from_env();
    println!("{:?}", a);
}

The main downside is that it won't handle subcommand flags. However there are two ways you could handle this case. First, you could copy the arguments into TopLevel with:

struct TopLevel {
    #[argh(option)]
    /// how many x
    x: usize,

    #[argh(subcommand)]
    nested: Option<MySubCommandEnum>,
}

but this has the downside that you can specify these flags even when we're calling the two subcommand. The other way is that you can capture the unknown arguments with:

struct TopLevel {
    #[argh(positional)]
    /// unknown arguments
    args: Vec<String>,

    #[argh(subcommand)]
    nested: Option<MySubCommandEnum>,
}

argh will first try to populate the subcommands before handling positional arguments, so if nested was None, you could then re-parse the args into whatever default subcommand you want. The downside with this is that the type system doesn't guarantee that you can't get a args values with a nested value being specified at the same time.

erickt · 2022-06-22T22:25:00Z

README.md

+#[derive(FromArgs, PartialEq, Debug)]
+/// Top-level command.
+struct TopLevel {
+    #[argh(subcommand, default = "one")]


Would using the string name of the subcommand be brittle? Since we wouldn't get an error if the variant was renamed. Instead, did you consider either:

Call a method to create the default subcommand

Copy the pattern of from_str_fn and have something like #[argh(subcommand, default(MySubCommandEnum::One)].

Adding a #[argh(default)] on the subcommand variant, with something like this:

#[derive(FromArgs)] struct TopLevel { #[argh(subcommand)] nested: MySubCommandEnum, } #[derive(FromArgs)] #[argh(subcommand)] enum MySubCommandEnum { #[argh(default)] One(SubCommandOne), Two(SubCommandTwo), } ...

I did put some thoughts into it and I think the 3rd option is a bit opaque, i.e you'd have to find the subcommand to find the default and I'd like it to specified on the higher level struct.

Another alternative (Option 1) is to force trail impl of the subcommand but I think that is too much burden on the end user but it would confine the command to a common interface.

I think option 2 would be the best choice here and it also turn a mislabeled default into a proper compile time error.

erickt · 2022-06-22T22:26:24Z

argh/src/lib.rs

+    /// Just like `from_args`, but with an additional parameter to specify whether or not
+    /// to print the first line of command_usage.
+    /// Internal use only.
+    fn from_args_show_command_usage(


Adding a new method without a default implementation is a breaking change. Is there another way we could do this?

Note that even if we do decide to make a breaking change, I'd prefer us not have internal use only methods like this. We have users that manually implement these traits rather than going through argh_derive, and it'd be unclear how those users should implement these methods. It'd be somewhat better to have from_args_show_command_usage call from_args by default, but that's still a little ugly.

Indeed, if that is the case then this would be a breaking change and the non-breaking/easiest way is what you have mentioned.

erickt · 2022-06-22T22:26:36Z

argh/src/lib.rs

+    /// Just like `redact_arg_values`, but with an additional parameter to specify whether or not
+    /// to print the first line of command_usage.
+    /// Internal use only.
+    fn redact_arg_values_show_command_usage(


Also a breaking change.

erickt · 2022-06-22T23:00:58Z

argh/tests/lib.rs

+fn option_subcommand_with_default() {
+    #[derive(FromArgs, PartialEq, Debug)]
+    /// Top-level command.
+    struct TopLevel {


what happens if we have a name collision between top-level fields, and the default subcommand field? For example, say we change this struct to:

struct TopLevel { #[argh(option)] /// how many toplevel x x: String, nested: Option<MySubCommandEnum>, }

we'd collide on --x. I'm guessing the TopLevel field would probably take precedence. Ideally this would be an error, but I don't think we have enough info to enforce this in argh_derive.

qhua948 · 2022-06-22T23:16:58Z

@erickt This function was needed for https://fuchsia-review.googlesource.com/c/fuchsia/+/688982

in this case, we want to drop the support for

ffx log --filter ... dump
ffx log --filter .. watch

But we still wish to retain the behavior of

ffx log == ffx log watch (The default command)
and
ffx log --filter == ffx log watch --filter

See also: https://fuchsia-review.googlesource.com/c/fuchsia/+/688982/comments/211fa14b_bfe6b463

erickt · 2022-06-22T23:29:04Z

@qhua948 Since it typically takes some time to hash out a feature like this, to unblock you on https://fuchsia-review.googlesource.com/c/fuchsia/+/688982, I think you could use this trick to get this to work:

#[derive(FromArgs)]
#[argh(
  subcommand,
  description = "Display logs from a target device. Defaults to `watch` if no subcommand is specified",
  note = "...",
)]
struct LogCommand {
    #[argh(positional)]
    /// unknown arguments
    args: Vec<String>,

    #[argh(subcommand)]
    sub_command: Option<LogSubCommand>,
}

...

pub async fn log_impl<W: std::io::Write>(
    diagnostics_proxy: DiagnosticsProxy,
    rcs_proxy: Option<RemoteControlProxy>,
    log_settings: Option<LogSettingsProxy>,
    mut cmd: LogCommand,
    writer: &mut W,
    opts: LogOpts,
) -> Result<()> {
    if cmd.sub_command.is_none() {
        cmd.sub_command = LogWatchCommand::from_args(&cmd.args)?;
    }
}

qhua948 · 2022-06-22T23:30:40Z

Side note, #15 is tangentially related

miguelfrde · 2022-06-22T23:35:38Z

What @qhua948 said :) I'll add some additional context.

@erickt the example you wrote is basically the approach we are following on ffx log:

struct TopLevel {
    #[argh(option)]
    /// how many x
    x: usize,

    #[argh(subcommand)]
    nested: Option<MySubCommandEnum>,
}

However, the fact that the dump flags are coming before the dump command is confusing folks as they expect the flags to come after the command: https://fxbug.dev/98857 and I agree it's quite a confusing behavior:

$ ffx log dump --filter foo
Unrecognized argument: --filter

See 'ffx help <command>' for more information on a specific command.

$ ffx log --filter foo dump
// works

ffx log watch is the most common way of using this command and users are used to just running ffx log and see the logs stream, hence why we want the behavior to be: default to watch when no sub-command is specified.

Now we are introducing ffx log list for which the flags that apply to dump and watch don't apply anymore. https://fxbug.dev/68154 has the context on this new command.

Going with the Vec<String> approach sgtm for now, but this generally seems like a useful feature for argh to have if folks are willing to iterate on it we can refactor the workaround Vec<String> later.

Regarding flattening, that's another related issue that we are solving at the moment by duplicating the struct defining the flags for both watch and dump and then converting to a SharedOpts struct.

qhua948 · 2022-06-23T13:30:17Z

@erickt @miguelfrde

The trick won't really work because

options gets parsed by the top level command regardless and will not get a chance to be populated into unknown args unless -- is encountered.
The current implementation will perform an early exit if no sub command is matched, even when end of opt is supplied

Added a new optional attribute field to `argh::subcommand(default = "xxx")`. This command allows user to specify a default subcommand for a higher level subcommand. Tweaked output for default subcommands. Addressed various PR feedback.

qhua948 · 2022-06-24T05:48:24Z

@erickt I have implemented alternative style for defaults

#[derive(FromArgs)]
struct TopLevel {
    #[argh(subcommand), default = "SubCommandOne"]
    nested: MySubCommandEnum,
}

#[derive(FromArgs)]
#[argh(subcommand)]
enum MySubCommandEnum {
    #[argh(default)]
    One(SubCommandOne),
    Two(SubCommandTwo),
}

When the default is specified wrongly, it will be a compile time error.

miguelfrde reviewed Jun 13, 2022

View reviewed changes

argh/src/lib.rs Outdated Show resolved Hide resolved

miguelfrde suggested changes Jun 13, 2022

View reviewed changes

argh/src/lib.rs Show resolved Hide resolved

argh/tests/lib.rs Show resolved Hide resolved

qhua948 mentioned this pull request Jun 14, 2022

[Optimisation] Stop re-parsing of commands for default sub commands #130

Open

qhua948 force-pushed the master branch 3 times, most recently from 53ed645 to 1e52d3b Compare June 14, 2022 00:43

miguelfrde reviewed Jun 14, 2022

View reviewed changes

qhua948 force-pushed the master branch 2 times, most recently from 4d50cb1 to d4f6952 Compare June 18, 2022 04:14

erickt requested changes Jun 22, 2022

View reviewed changes

qhua948 force-pushed the master branch from d4f6952 to 4807d97 Compare June 23, 2022 09:48

qhua948 force-pushed the master branch from 4807d97 to 513a281 Compare June 24, 2022 05:45

Implement default subcommand.

565b6f6

Added a new optional attribute field to `argh::subcommand(default = "xxx")`. This command allows user to specify a default subcommand for a higher level subcommand. Tweaked output for default subcommands. Addressed various PR feedback.

qhua948 force-pushed the master branch from 513a281 to 565b6f6 Compare June 24, 2022 05:46

qhua948 requested a review from erickt June 24, 2022 05:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement default subcommand. #129

Implement default subcommand. #129

qhua948 commented Jun 13, 2022

miguelfrde commented Jun 13, 2022

miguelfrde Jun 14, 2022

qhua948 Jun 15, 2022

qhua948 Jun 15, 2022

erickt left a comment

erickt Jun 22, 2022

qhua948 Jun 22, 2022

qhua948 Jun 23, 2022

erickt Jun 22, 2022

qhua948 Jun 22, 2022

erickt Jun 22, 2022

erickt Jun 22, 2022

qhua948 commented Jun 22, 2022

erickt commented Jun 22, 2022

qhua948 commented Jun 22, 2022

miguelfrde commented Jun 22, 2022

qhua948 commented Jun 23, 2022 •

edited

qhua948 commented Jun 24, 2022 •

edited

Implement default subcommand. #129

Are you sure you want to change the base?

Implement default subcommand. #129

Conversation

qhua948 commented Jun 13, 2022

miguelfrde commented Jun 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erickt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qhua948 commented Jun 22, 2022

erickt commented Jun 22, 2022

qhua948 commented Jun 22, 2022

miguelfrde commented Jun 22, 2022

qhua948 commented Jun 23, 2022 • edited

qhua948 commented Jun 24, 2022 • edited

qhua948 commented Jun 23, 2022 •

edited

qhua948 commented Jun 24, 2022 •

edited