fix(help): Render partially optional values with [] #4910
Conversation
fabianfreyer
force-pushed
the
fix/issue-4847
branch
from
e0a43cd
to
11c0ae1
Compare
clap_builder/src/builder/arg.rs
Outdated
| let arg_name = if self.is_positional() && (num_vals.min_values() == 0 || !required) { | ||
| let all_optional = self.get_min_vals() == 0; | ||
| let arg_name = if match self.is_positional() { | ||
| true => num_vals.min_values() == 0 || !required, |
There was a problem hiding this comment.
So yes, it seems like those aren't really correct at the moment as well. To a certain extent, the same logic applies, and I've got a patch that unifies handling, but it seems to break a lot of tests (which arguably might be wrong in their current state, but I'm not sure about). I think in its current form, this PR is already an improvement, and I'm happy to create another one to address partially optional positionals, but I think those will need a lot more care to get right.
There was a problem hiding this comment.
I definitely understand the importance of deferring work but this feels odd to me.
We are going from positionals having an extra feature (showing which values are optional) and extending it to named arguments but with a bug fixes not contained in the original code, relying on conditionals to isolate the two different implementations. Granted, if we have to have split implementations because of different requirements, I can understand but nothing is indicating that is the case here and if it is, then we should be documenting that.
There was a problem hiding this comment.
My main reason to be apprehensive of touching the existing code for positionals is that there are a ton of tests which break once I do that. Granted, these tests may need to be updated, but I'm honestly not sure.
There was a problem hiding this comment.
If this were a new feature on its own, I would sign off. However, bifurcating a feature like this is a blocker.
There was a problem hiding this comment.
Also, if there is a lot of churn from changing positionals, I likely would want that in a PR before this one so this remains easy to review
There was a problem hiding this comment.
I'll try to figure out what needs to be done there, since I currently don't really understand what's wrong with positionals. I think I don't really understand how positionals with multiple, partially optional values would work.
There was a problem hiding this comment.
Nevertheless, I think the conditional for handling those differently than named arguments will remain necessary (and was there before this PR, implicitely, through if self.is_positional(). One main difference in requirements is rendering of the --arg[=<value>] case.
fabianfreyer
force-pushed
the
fix/issue-4847
branch
2 times, most recently
from
3ad1fb3
to
f92d34b
Compare
fabianfreyer
force-pushed
the
fix/issue-4847
branch
from
f92d34b
to
0be886f
Compare
clap_builder/src/builder/arg.rs
Outdated
| let value_is_required = match self.is_positional() { | ||
| true => required && (num_vals.min_values() != 0), | ||
| false => { |
There was a problem hiding this comment.
Sorry, I lost track of this earlier, but why a match on a bool? I would assume if self.is_positional would be a more obvious
There was a problem hiding this comment.
I'll push an if...else version!
fabianfreyer
force-pushed
the
fix/issue-4847
branch
from
0be886f
to
5bfbe2b
Compare
Fixes: #4847