Like with #2903, we are depending on more features than needed, forcing
them onto users. This also impacts our testing since we aren't truly
getting minimal test runs (see #2976).
When supporting multiple occurrences for positional arguments in #2804,
I added some tests to cover this but apparently simpler cases fail
despite those more complicated cases being tested.
This adds more multiple-occurrences tests for positional arguments,
fixes them, and in general equates multiple values with occurrences for
positional arguments as part of #2692. There are a couple more points
for consideration for #2692 for us to decide on once this unblocks them
(usage special case in #2977 and how subcommand help should be handled).
I fully admit I have not fully quantified the impact of all of these
changes and am heavily relying on the quality of our tests to carry this
forward.
I noticed this while investigating #2692. Since we are making
multiple-occurrences a thing for positional arguments, this allows us to
remove a special case.
Another way to look at this is that we should make the default whatever
we do for dervies (#1772). I'm going to propose we make the derive
always turn `Vec<i32>` into multiple occurences and not multiple values
(with users being able to change it through attributes), but that is an
in-work proposal and not decided yet.
BREAKING CHANGE: `Arg::from(...)` will now use `multiple_occurrences`
for a positional `...`, rather than `multiple_values`.
In looking at multiple occurrences and values for issues like #2692, I
noticed that `...` can mean both multiple values and multiple
occurrences, like before we split them.
Pros
- No syntax change with clap3
Cons
- All the reasons we split `multiple` into two
Uncertain
- I originally started this as part of another branch but I lost track
if something depended on this. I'll have to do more digging
BREAKING CHANGE: If `--opt [val]...` was meant for
- only multiple occurrences, see `[opt]... --opt [val]`
- both multiple occurrences and values, see `[opt]... --opt [val]...`
Besides being more explicit / clear, this makes it easier to experiment
with tweaking the logic and keeping the test updates minimal, so the
change of behavior stands out more.
Wow, I'm having a hard time summarizing this for the summary.
When we code-gen Settings using `impl_settings`, we have no control over
whether one instance of a Settings will use some or all functions. I
have a fix that removes the one `ArgSettings::unset`, causing warnings
for `ArgSettings` despite needing it for `AppSettings`.
So I'm making it less dependent on how each instantiation uses it.
`Parser::parse_from` will call `exit` on failure and we don't just lose
backtrace information but we don't even know which of the tests running
in parallel panicked. I ran into this when experimenting with
`clap_derive` and I couldn't tell what actually failed.
So let's switch to `Parse::try_parse_from`.
Errors went from:
```
test option_option ... ok
error: Found argument 'bar' which wasn't expected, or isn't valid in this context
USAGE:
clap_derive [OPTIONS]
For more information try --help
error: test failed, to rerun pass '--test arg_enum'
```
To:
```
test option_option ... ok
test variant_with_defined_casing ... ok
test skip_variant ... ok
test default_value ... ok
test vector ... FAILED
test option_vector ... ok
failures:
---- vector stdout ----
thread 'vector' panicked at 'called `Result::unwrap()` on an `Err` value: Error { message: Formatted(Colorizer { use_stderr: true, color_when: Auto
, pieces: [("error:", Some(Red)), (" ", None), ("Found argument '", None), ("bar", Some(Yellow)), ("' which wasn't expected, or isn't valid in this
context", None), ("\n\n", None), ("USAGE:\n clap_derive [OPTIONS]", None), ("\n\nFor more information try ", None), ("--help", Some(Green)), ("
\n", None)] }), kind: UnknownArgument, info: ["bar"], source: None, backtrace: Backtrace }', clap_derive/tests/arg_enum.rs:388:56
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
vector
test result: FAILED. 15 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
error: test failed, to rerun pass '--test arg_enum'
```
Technically, this also fixes a bug with an extra newline when building
with `debug` but that is a pretty minor cosmetic issue. It is unclear
if `backtrace` has a trailing newline or not, so I left that as-is.
Before, `Error::exit` didn't provide `WaitOnError`, requiring each call
site to duplicate half of `Error::exit`s behavior to get it. This
hadn't been done for errors raised by derive-generated code. Ideally,
these errors never happen but all the same, having this consistent would
be good.
This moves knowledge of `WaitOnError` to `Error` (including through
`Error::format`) so `Error::exit` can wait. Now all of the callers to
`.exit` get a consistent experience without duplication.
While #2938 made a lot of `Error` fields optional for less churn-heavy
modifications, I made this new field required to minimize the risk of an
raise site forgetting to set it.
2966: Add new `App::mut_args` for mutating all arguments r=pksunkara a=repi
Co-authored-by: Johan Andersson <repi@repi.se>
Co-authored-by: Johan Andersson <johan@embark-studios.com>
2961: Pin the zola version used to deploy the site r=epage a=pksunkara
2965: fix(error): Never show unrequested color r=pksunkara a=epage
Co-authored-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
Co-authored-by: Ed Page <eopage@gmail.com>
If the user prints a raw error, it may include color even if the user
turned it off at runtime. Now we'll be more conservative and never show
color for raw errors.
This is a follow up to #2943; apparently I had missed some cases.
This is to help verify behavior added in #2943. We separated the error
raising site from the error formatting site and this verifies that the
formatting actually happens.
We prefer Settings to always be off by default, so when we change a
default, we have to rename.
This adds back in the now-default settings with deprecation messages to
help the user know how things now work.
Unfortunately, there is no way to notify the user that the default they
relied on has changed. This also doesn't help us when the change in
behavior is more than just an inverting, like `InvalidUtf8` or when a
setting mapped to multiple bits.
This partiall reverts commit efeb02cd34,
bringing back the `AppSettins::Color*` and making them the backing store
for `App::color`, to help ease the transition from clap2->3.
Once we remove these deprecated settings, we might want to keep this
backing store to save on memory.
This is a part of #2617
This brings back the old name of settings, just deprecated. Since they
all map to the same bits in the bit field, this should work for
`setting` and `is_set`. The only thing this lacks is being able to do
equality across variants, whcih seems like a minority case.
Removed settings have some extra care abouts that we'll need to look
into separately.
This is a part of #2617
