Implement default subcommand.

[qhua948]

#57

[miguelfrde]

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.

[qhua948]

@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]

@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]

Side note, https://github.com/google/argh/issues/15 is tangentially related

[miguelfrde]

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]

@erickt @miguelfrde

The trick won't really work because

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

[qhua948]

@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.