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(validator): Ignore defaults for requireds #3793

Merged
merged 2 commits into from
Jun 6, 2022
Merged

Conversation

epage
Copy link
Member

@epage epage commented Jun 6, 2022

This is a follow up to #3420. Its easy to overlook this because it is only
useful for the conditionals (we actually prevent applying unconditional
defaults to unconditional requireds). This became apparent with the
increased use of defaults with SetTrue.

As always, there is the question of when is a bug fix a breaking change.
I'm going to consider this safe since we prevent some instances of this
from even happening and we already did #3420 and this is in line with
those.

epage added 2 commits June 6, 2022 10:15
This is a follow up to clap-rs#3420.  Its easy to overlook this because it is only
useful for the conditionals (we actually prevent applying unconditional
defaults to unconditional requireds).  This became apparent with the
increased use of defaults with `SetTrue`.

As always, there is the question of when is a bug fix a breaking change.
I'm going to consider this safe since we prevent some instances of this
from even happening and we already did clap-rs#3420 and this is in line with
those.
@epage epage merged commit 955f8b6 into clap-rs:master Jun 6, 2022
@epage epage deleted the required branch June 6, 2022 16:32
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
zydou pushed a commit to zydou/arti that referenced this pull request Nov 9, 2022
This removes the last cargo audit override (for the unmaintained
ansi_term).

Don't mark options as required when they have default values:
see <clap-rs/clap#3793>.
crowlKats pushed a commit to denoland/deno that referenced this pull request Aug 13, 2023
…ags` is present in `deno run` (#20145)

Fix #20022, fix #19627 (duplicate)

#17333 upgraded clap from version 3.1 to version 4. clap version 3.2.0
(intentionally) broke a behavior that deno was relying on to make `deno
run --v8-flags=--help` work without specifying a file, see
clap-rs/clap#3793. The workaround was to make the script argument
required _unless_ `--v8-flags` is present. This broke the expectation
that all successfully parsed `run` commands have the script argument
set, leading to the panic on
`matches.remove_many::<String>("script_arg").unwrap()`.

Clap, as far as I was able to find out, does not currently offer a neat
solution to this problem. This PR adds logic to create and return a
custom clap error when a parsed run command does not have the script
argument.

I added an appropriate test.
littledivy pushed a commit to littledivy/deno that referenced this pull request Aug 21, 2023
…ags` is present in `deno run` (denoland#20145)

Fix denoland#20022, fix denoland#19627 (duplicate)

denoland#17333 upgraded clap from version 3.1 to version 4. clap version 3.2.0
(intentionally) broke a behavior that deno was relying on to make `deno
run --v8-flags=--help` work without specifying a file, see
clap-rs/clap#3793. The workaround was to make the script argument
required _unless_ `--v8-flags` is present. This broke the expectation
that all successfully parsed `run` commands have the script argument
set, leading to the panic on
`matches.remove_many::<String>("script_arg").unwrap()`.

Clap, as far as I was able to find out, does not currently offer a neat
solution to this problem. This PR adds logic to create and return a
custom clap error when a parsed run command does not have the script
argument.

I added an appropriate test.
littledivy pushed a commit to denoland/deno that referenced this pull request Aug 21, 2023
…ags` is present in `deno run` (#20145)

Fix #20022, fix #19627 (duplicate)

#17333 upgraded clap from version 3.1 to version 4. clap version 3.2.0
(intentionally) broke a behavior that deno was relying on to make `deno
run --v8-flags=--help` work without specifying a file, see
clap-rs/clap#3793. The workaround was to make the script argument
required _unless_ `--v8-flags` is present. This broke the expectation
that all successfully parsed `run` commands have the script argument
set, leading to the panic on
`matches.remove_many::<String>("script_arg").unwrap()`.

Clap, as far as I was able to find out, does not currently offer a neat
solution to this problem. This PR adds logic to create and return a
custom clap error when a parsed run command does not have the script
argument.

I added an appropriate test.
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