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

fix!: Remove Arg::rwquire_value_delimiter #4026

Merged
merged 1 commit into from
Aug 4, 2022
Merged

Conversation

epage
Copy link
Member

@epage epage commented Aug 4, 2022

In clap v3, require_value_delimiter activated an alternative parse
mode where

  • multiple_values meant "multiple values within a single arg"
  • number_of_values having no parse impact, only validation impact
  • value_names being delimited values

For unbounded number_of_values, this is exactly what value_delimiter
provides. The only value is if someone wanted value_name to be
<file1>,<file2>,... which can be useful and we might look into adding
back in.

Alternatively, this could be used for cases like key-value pairs but
that has issues like not allowing the delimiter in the value which might
be ok in some cases but not others. We already instead document that
people should instead use ValueParser for this case.

In removing this, we remove points of confusion at how the different
multiple values and delimited value calls interact with each other. I
know I would set require_value_delimiter(true).multiple_values(true)
when it turns out all I needed was value_delimiter(',').

This also reduces the API surface area which makes it easier to discover
what features we do provide.

While this isn't big, this is also yet another small step towards
reducing binary size and compile times.

This is part of #2688

In clap v3, `require_value_delimiter` activated an alternative parse
mode where
- `multiple_values` meant "multiple values within a single arg"
- `number_of_values` having no parse impact, only validation impact
- `value_names` being delimited values

For unbounded `number_of_values`, this is exactly what `value_delimiter`
provides.  The only value is if someone wanted `value_name` to be
`<file1>,<file2>,...` which can be useful and we might look into adding
back in.

Alternatively, this could be used for cases like key-value pairs but
that has issues like not allowing the delimiter in the value which might
be ok in some cases but not others.  We already instead document that
people should instead use `ValueParser` for this case.

In removing this, we remove points of confusion at how the different
multiple values and delimited value calls interact with each other.  I
know I would set `require_value_delimiter(true).multiple_values(true)`
when it turns out all I needed was `value_delimiter(',')`.

This also reduces the API surface area which makes it easier to discover
what features we do provide.

While this isn't big, this is also yet another small step towards
reducing binary size and compile times.
@epage epage merged commit dc5ce00 into clap-rs:master Aug 4, 2022
@epage epage deleted the require branch August 4, 2022 05:48
@gibfahn
Copy link
Contributor

gibfahn commented Dec 15, 2022

So for migrating clap v3 -> v4 require_value_delimiter() can be removed, as its behaviour is the default if value_delimiter() is set.

(unsure if this was true for v3).

So before I had:

      #[clap(
          short, long,
          // You can pass multiple components.
          multiple_values(true),
          // You can pass this option multiple times, e.g. `-t foo -t bar`.
          multiple_occurrences(true),
          // You can use a comma to separate values, e.g. `-t foo,bar`.
          use_value_delimiter(true),
          // You must use a comma to separate values not a space, so can't do `-t foo bar`
          require_value_delimiter(true),
      )]
      pub(crate) tasks: Option<Vec<String>>,

And now that becomes:

    #[clap(short, long, value_delimiter = ',')]
    pub(crate) tasks: Option<Vec<String>>,

(leaving this here in case anyone else followed the issues here as part of a v3 -> v4 migration)

EDIT: Also this is way simpler than what was there before, so thanks for that @epage !

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

Successfully merging this pull request may close these issues.

2 participants