We had some tests for this but not sufficient obviously. The problem is
we were tweaking the positional argument counter when processing flags
and not just positional arguments. Delaying it until after flags seems
to fix this.
Fixes#3959
This fixes a bug introduced in 4a694f3592
when we were trying to move away from presence checks via occurrences.
I switched it to the common type of presence check but really what we
want is a highest-precedence check.
Fixes#3872
Someone should not reasonably expect a coun flag to go up to billions,
millions, or even thousands. 255 should be sufficient for anyone,
right?
The original type was selected to be consistent with
`ArgMatches::occurrences_of` but that is also used for tracking how
many values appear which can be large with `xargs`.
I'm still conflicted on what the "right type" is an wish we could
support any numeric type. When I did a search on github though, every
case was for debug/quiet flags and only supported 2-3 occurrences,
making a `u8` overkill.
This came out of a discussion on #3792
This mostly exist for
- Knowing of the value came from the command-line but we now have
`ArgMatches::source`
- Counting the number of flags but we now have `ArgAction::Count`
This shouldn't be needed anymore now that this is effectively the new
behavior for the non-deprecated actions.
This was briefly talked about in
https://github.com/clap-rs/clap/discussions/2627 but I wasn't familiar
enough with the implementation to know how safe it is. Now, maintainrs
and users can be more confident because they are explicitly opting into
it.
See also #3795
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.
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
This round out the new style actions and allow us to start deprecating
occurrences.
As part of an effort to unify code paths, this does change flag parsing
to do splits. This will only be a problem if the user enables splits
but we'll at least not crash. Once we also address #3776, we'll be able
to have envs all work the same.
If we felt this was important long-term, we should fix this outside of
the Action. Since we might be changing up occurrences (#3772), we can
probably get away with a hack.
This changes how occurrences and values are grouped for multiple values.
Today, it appears as a bug. If we move forward with #3772, then this
can make sense.
I wrote these tests expecting to highlight a bug but it turns out things
were structured just right to not exhibit it. The fact that the code
looks like its broken is a problem, so I restructured it (put it first,
changed the source) so it doesn't look suspicious anymore.
We were independently starting occurrences and starting value groups.
Now we do them at the same time.
COMPATIBILITY: This changes us from counting occurrences per positional
when using `multiple_values` to one occurrence. This is user visible
and tests were written against it but it goes against the documentation
and doesn't quite make sense.
When to show usage? We are currently mixed about it. For `validator`,
we didn't show it at all. Sometimes we show the used arguments and
sometimes we don't.
With `ValueParser`, I ran into the problem that we weren't showing the
used arguments like we had previously in some cases. In deciding how to
solve this, I went with the simplest route for now and removed it as the
usage likely doesn't add much context to help people solve their
problem, more so the recommendation for help. We'll see how the
feedback is on this and adjust.
`multicall` allows you to have one binary expose itself as multiple
programs, like busybox does. This also works well for user clap for
parsing REPLs.
Fixes#2861
Unfortunately, we can't track using a `ValueParser` inside of `Command`
because its `PartialEq`. We'll have to wait until a breaking change to
relax that.
Compatibility:
- We now assert on using the wrong method to look up defaults. This
shouldn't be a breaking change as it'll assert when getting a real
value.
- `values_of`, et al delay panicing on the wrong lookup until the
iterator is being processed.
With us moving the required de-duplication up a level, it made this
check redundant. By removing this check, we're more likely to have an
item in the `incls` which forces a smart usage and reduces the chance of
an `[ARGS]` or `[OPTIONS]`, so a couple of tests changed.
Gave up trying to decipher the existing logic for safe ways to
de-duplicate manually and switched to an `IndexSet` to enforce only one
of each argument exists.
Fixes#3556
This is a step towards #3309. We want to make longs and long aliases
more consistent in how they handle leading dashes. There is more
flexibility offered in not stripping and it matches the v3 short
behavior of only taking the non-dash form. This starts the process by
disallowing it completely so people will catch problems with it and
remove their existing leading dashes. In a subsequent breaking release
we can remove the debug assert and allow triple-leading dashes.
`Arg::exclusive` is just another way of defining conflicts, so a
present-exclusive arg should override required like other conflicts.
Instead of going through the message of enumerating all other arguments
as exclusive, I shortcutted it and special case exclusive in the
required check like we do with conflicts. The big downside is the
implicit coupling between the code paths rather than having a consistent
abstraction for covering conflicts.
This isn't a breaking change because if someone defined an exclusive arg
as a sibling to a required arg, the exclusive arg could never be used,
it always errored, and so no valid application can be written with it.
Fixes#3595
This is a step towards #992. When help renders the application name, it
uses the `bin` template variable which is just the `bin` name with
spaces converted to ` `. While having `app.exe sub` makes sense,
`app.exe-sub` does not.
To get around needing this for usage, we've created a `display_name`
field that is fairly similar but
- The root name is the `name` and not `bin_name`
- We always join with `-`
This means that the derived `bin_name` will only show up in usage.
For now, the default template has not been updated as that is a minor
compatibility change and should be in a minor release, at least. I was
worried this would be a full breaking change. The main case I was
worried about was cargo subcommands but our tests show they should just
work.
By removing all arguments, we've switched from an "unrecognized
argument" error to a "unrecognized subcommand" error. While the wording
has room for improvement, its at least progress on #2862.
Before, if two arguments were required *and* overrode each other, then
`cmd --opt=1 --other=2` succeded but `cmd --other=2` failed despite
ignoring `--opt=1`. Requiring `--opt=1` to be present but unavailable
doesn't help anyone and makes the behavior less predictable.
Now both commands will have the same behavior.
`Command::_build_all` started as an internal function for
`clap_complete` as a stopgap until #2911. Overtime, we've been finding
more cases where this function needs to be called, so now we're going to
fully embrace it until #2911 so people aren't scrared off by the hidden
implementation from using it.
This was inspired by #3602
Comptibility: Though this adds a deprecation which we general reserve
for minor or major versions, this is enough of a corner case that I'm
fine doing this in a patch release.
`-h` (short help) still shows the same.
This gates it behind an `unstable-v4` feature flag to avoid disrupting users who set the help without knowing where all it shows up (particularly derive users where `ArgEnum` is automatically extracting the help).
Fixes#3312
Instead of just renaming it, I reconsidered what the API should look
like. A custom separator for author does not make sense positionally
but accepting a name, and defaulting it, does fit with what someone
would expect.
I removed the `_from_crate` suffix because it doesn't seem necessary.
We don't have this kind of naming for the derive. I feel it cleans
things up this way.
Now that we can use `SubcommandRequired |
ArgRequiredElseHelp`, this setting offers little value but requires we
track required subcommands with two different settings. Deprecating as
the cost is not worth the benefit anymore.
Issue #3280 will see the derive updated
Like was said in #2435, this is what people would expect.
While we should note this in a compatibility section in the changelog, I
do not consider this a breaking change since we should be free to adjust
the help output as needed. We are cautious when people might build up
their own content around it (like #3312) but apps should already handle
this with `--help` so this shouldn't be a major change.
We aren't offering a way for people to disable this, assuming people
won't need to. Longer term, we are looking at support "Actions" (#3405)
and expect those to help customize the flags. We'll need something
similar for the `help` subcommand.
Fixes#3440
This is a part of #2717
Some settings didn't get getters because
- They are transient parse settings (e.g. ignore errors)
- They get propagated to args and should be checked there
`is_allow_hyphen_values_set` is a curious case. In some cases, we only
check the app and not an arg. This seems suspicious.
For the derive API, you can only call `next_display_order` when dealing
with a flatten. Until we offer app attributes on arguments, the user can workaround with
this no-op flattens.
This is a part of #1807
This clarifies the intent and prepares for other functions doing the
same, like `next_display_order`. This will then open us to name
`subcommand_help_heading` and `display_order` similar.
The deprecation is waiting on 3.1.
This is part of #1807 and #1553.
This is inspired by cargo which allows you to run `cargo test --test`
and it will list the possible tests (obviously we can't support that atm
because that requires a lot of runtime processing). When we do have a
static list of possible values, we can at least show those.
Fixes#3320
For some errors, we use the unroll logic to get the list of required
arguments. The usage then does the same, but without a matcher. This
was causing the lists to not match.
As a side effect, this fixed an ordering issue where we were putting the
present arg after the not-present arg. I assume its because we ended up
reporting the items twice but the first time is correctly ordered and
gets precedence.
This was split out of #3020
When an Arg uses .min_values(0), that arg's value(s) are effectively
optional. This is conventionaly denoted in help messages by wrapping the
arg's values in square brackets. For example:
--foo[=value]
--bar [value]
This kind of argument can be seen in the wild in many git commands; e.g.
git-status(1).
Signed-off-by: Peter Grayson <pete@jpgrayson.net>
We were only tracking the last value source (default, env, cli, etc).
This works for args because they only come from one source. Groups
however can come from multiple sources and this was making us treat a
group with a default value as being completely from defaults despite
some values maybe being from the commandline.
We now track the highest precedence value for a group.
Fixes#3330
I thought I was adding a test in #3305. Maybe I considered the
clap_complete changes sufficient for this. Doing more `debug_assert`s
would be good also.
Unsetting `PropagateVersion` helps in some cases but we need to unset it
globally to override the global propagation.
Fixes#3310
This reverts commit 333b993481.
PR #1810's motivation was effectively "this is redundant with `\n`".
That is a fine motivation. Unfortunately, we don't have a way to force
a hard break in `clap_derive`. #2389 should help with this eventually
but we shouldn't hold up 3.0 to get that ready. So in the mean time, we
are restoring `{n}`.
We have #3230 for tracking the re-removal of it.
This allows two overriding args to interact with each other mid-line.
This was broken in 7673dfc / #1154. Before that, we duplicated the override
logic in several places.
Now we plug into the start of a new arg which allows us to do this
incrementally without making the logic complex or inefficient, thanks to
prior refactors.
Fixes#3217
The extra whitespace was targeted at machine processing for a subset of
users for a subset of runs of CLIs. On the other hand, there is a lot
of concern over the extra verbose output.
A user can set the help template for man, if desired. They can even do
something (env? feature flag?) to make it only run when doing man
generation. We also have #3174 in the works.
So let's focus on the end-user reading `--help`. People wanting to use
`help2man` have workarounds to do what they need.
Fixes#3096
We have two ways of fixing this
- Making `--help` work
- Don't put `--help` in the help output
For now, I went with the latter. I tried to make it clear what the
actual requirement is so we can pivot if needed.
Fixes#2892
This happens to also fix the interaction of `DisableHelpFlag` with the
help subcommand and complcations. I've added a test to help catch if we
break this by changing how we fixed the original issue.
Fixes#2724
These issues were reported against clap3. I've not tried to reproduce
these in clap2 to see if they should show up in the release notes.
I'm assuming we won't have a negative performance impact by removing
`impl Copy for Id` because the compiler would inline the `clone()`s and
turn them into copies.
Addresses problems from #3164
This is prep for moving the derive tests. Besides organizing the test
folder for each API, this should reduce link time at the cost of
re-compiling more when a test changes.
