Skip to content

Commit

Permalink
fix: when an argument requires a value and that value happens to matc…
Browse files Browse the repository at this point in the history
…h a subcommand name, its parsed as a value

**This commit contains a breaking change in order to fix a bug.**

The only users affected are those relying on the "bug." The bug is only in code that uses
subcommands, and parent commands with arguments accepting multiple values (positionals or options)
unrestrained *and* where a value may coincide with a subcommand's name.

Imagine a CLI with a positional argument `files` that accepts multiple values but no other conditions
or parameters (just `Arg::multiple(true)`), and a subcommand `log`

Prior to this commit, the following was valid:

```
$ prog file1 file2 file3 log
```

which would be parsed as:

 * files = file1, file2, file3
 * subcommand = log

However, if there was a file named `log` the subcommand isn't callable.

The CLI should be designed in a way that either limits number of values so that clap knows
when `files` is done and can then look for subcommands, or other AppSettings which change
the behavior.

Possible fixes are `Arg::number_of_values`, `Arg::max_values`,
`AppSettings::ArgsNegateSubcommands`, `Arg::require_equals`,
`AppSettings::SubcommandsNegateArgs`, and more.

This *does not* affect CLIs which contain other arguments between the unrestrained multiple
value args, and the any subcommands. Such as if our example above also had a flag `-f`

The following would be parsed the same before and after this commit.

```
$ prog file1 file2 file3 -f log
```

This is because upon reaching the flag `-f`, clap stops parsing `files`.

No other breaking changes were made.

---

When using args with `multiple(true)` on options or positionals (i.e. those args that
accept values) and subcommands, one needs to consider the posibility of an argument value
being the same as a valid subcommand. By default `clap` will parse the argument in question
as a value *only if* a value is possible at that moment. Otherwise it will be parsed as a
subcommand. In effect, this means using `multiple(true)` with no additional parameters and
a possible value that coincides with a subcommand name, the subcommand cannot be called
unless another argument is passed first.

As an example, consider a CLI with an option `--ui-paths=<paths>...` and subcommand `signer`

The following would be parsed as values to `--ui-paths`.

```
$ program --ui-paths path1 path2 signer
```

This is because `--ui-paths` accepts multiple values. `clap` will continue parsing values
until another argument is reached and it knows `--ui-paths` is done.

By adding additional parameters to `--ui-paths` we can solve this issue. Consider adding
`Arg::number_of_values(1)` as discussed above. The following are all valid, and `signer`
is parsed as both a subcommand and a value in the second case.

```
$ program --ui-paths path1 signer
$ program --ui-paths path1 --ui-paths signer signer
```

Closes #1031
  • Loading branch information
kbknapp committed Oct 24, 2017
1 parent b399ee2 commit 0c223f5
Showing 1 changed file with 15 additions and 10 deletions.
25 changes: 15 additions & 10 deletions src/app/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -848,17 +848,22 @@ impl<'a, 'b> Parser<'a, 'b>
if !self.is_set(AS::TrailingValues) {
// Does the arg match a subcommand name, or any of it's aliases (if defined)
{
let (is_match, sc_name) = self.possible_subcommand(&arg_os);
debugln!("Parser::get_matches_with: possible_sc={:?}, sc={:?}",
is_match,
sc_name);
if is_match {
let sc_name = sc_name.expect(INTERNAL_ERROR_MSG);
if sc_name == "help" && self.is_set(AS::NeedsSubcommandHelp) {
self.parse_help_subcommand(it)?;
match needs_val_of {
ParseResult::Opt(_) | ParseResult::Pos(_) =>(),
_ => {
let (is_match, sc_name) = self.possible_subcommand(&arg_os);
debugln!("Parser::get_matches_with: possible_sc={:?}, sc={:?}",
is_match,
sc_name);
if is_match {
let sc_name = sc_name.expect(INTERNAL_ERROR_MSG);
if sc_name == "help" && self.is_set(AS::NeedsSubcommandHelp) {
self.parse_help_subcommand(it)?;
}
subcmd_name = Some(sc_name.to_owned());
break;
}
}
subcmd_name = Some(sc_name.to_owned());
break;
}
}

Expand Down

0 comments on commit 0c223f5

Please sign in to comment.