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(parser): Provide default value for Actions #3786

Merged
merged 1 commit into from
Jun 3, 2022
Merged

Conversation

epage
Copy link
Member

@epage epage commented Jun 3, 2022

Actions were inspired by Python and Python does not implicitly default
any field when an action is given. From a Builder API perspective, this
seemed fine because we tend to focus the Builder API on giving the user
all information so they can make their own decisions. When working on
the Derive API, this became a problem because users were going to have
to migrate from an implied default to an explicit default when a common
default is good enough most of the time. This shouldn't interfere with
Builder users getting more details when needed.

This also highlighted two problems

  • We set the index for defaults
  • We don't debug_assert when applying conditional requirements with a
    default present

Actions were inspired by Python and Python does not implicitly default
any field when an action is given.  From a Builder API perspective, this
seemed fine because we tend to focus the Builder API on giving the user
all information so they can make their own decisions.  When working on
the Derive API, this became a problem because users were going to have
to migrate from an implied default to an explicit default when a common
default is good enough most of the time.  This shouldn't interfere with
Builder users getting more details when needed.

This also highlighted two problems
- We set the index for defaults
- We don't debug_assert when applying conditional requirements with a
  default present
@epage epage merged commit 8dd7d64 into clap-rs:master Jun 3, 2022
@epage epage deleted the default branch June 3, 2022 15:24
epage added a commit to epage/clap that referenced this pull request Jun 6, 2022
This is the derive support for clap-rs#3774 (see also clap-rs#3775, clap-rs#3777)

This combined with `value_parser` replaces `parser`.  The main
frustration with this is that `ArgAction::Count` (the replacement for
`parse(from_occurrences)` must be a `u64`.  We could come up with a
magic attribute that is meant to be the value parser's parsed type.  We
could then use `TryFrom` to convert the parsed type to the user's type
to allow more.  That is an exercise for the future.  Alternatively, we
have clap-rs#3792.

Prep for this included
- clap-rs#3782
- clap-rs#3783
- clap-rs#3786
- clap-rs#3789
- clap-rs#3793
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.

1 participant