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

value_of_t() is difficult to use with optional arguments #2505

Closed
2 tasks done
Tracked by #2683
dimo414 opened this issue May 28, 2021 · 15 comments · Fixed by #3732
Closed
2 tasks done
Tracked by #2683

value_of_t() is difficult to use with optional arguments #2505

dimo414 opened this issue May 28, 2021 · 15 comments · Fixed by #3732
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-enhancement Category: Raise on the bar on expectations M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing

Comments

@dimo414
Copy link

dimo414 commented May 28, 2021

Please complete the following tasks

  • I have searched the discussions
  • I have searched the existing issues

Describe your use case

See #2453 for earlier discussion

The value_of_t() function notes "there are two types of errors, parse failures and those where the argument wasn't present", and suggests callers differentiate between the two by inspecting the ErrorKind in the Result.

Note these two cases reflect two different types of issues. Parse failures occur when the user passes an invalid string on the command line, and typically an application would e.exit() in response. Absent arguments occur when the user simply didn't pass anything at all, which for optional arguments* is expected and typical applications should proceed.

While it is possible to inspect the ErrorKind, doing so is not very ergonomic or intuitive. Here's one such implementation:

let arg: Option<Duration> = match value_t!(matches.value_of("arg"), humantime::Duration) {
    Ok(t) => Ok(Some(t.into())),
    Err(e) => {
        match e.kind {
            ::clap::ErrorKind::ArgumentNotFound => Ok(None),
            _ => Err(e),
        }
    }
}.unwrap_or_else(|e| e.exit());

It would be nice if Clap made parsing optional arguments easier, or at a minimum improved the documentation of value_of_t() to describe how callers are intended to work with optional arguments and ErrorKind.

* Note that Clap handles missing required() arguments before control reaches a value_of_t() call, so there's little reason for the application to reject an absent argument at this step.

Describe the solution you'd like

Ideally, I'd like to see value_of_t() handle these two cases more distinctly, such as by changing the return type to Result<Option<R>, Error>. This would better align with the return type of value_of() and allow callers to cleanly and clearly handle these two separate situations, e.g.:

let len: Option<u32> = matches.value_of_t("length").unwrap_or_else(|e| e.exit());

I don't know if this sort of breaking change would still be possible for Clap 3.0, but if you're open to it I'd gladly send a PR.

Alternatives, if applicable

  • Add a wrapper function that converts a Result<R, Error> into a Result<Option<R>, Error> if the ErrorKind is ArgumentNotFound; this would be less user-friendly than above but would be safe to add.
  • Clarify in the documentation of value_of_t() and siblings how callers are intended to handle these separate situations.

Additional Context

No response

@pksunkara pksunkara added C: matches C-enhancement Category: Raise on the bar on expectations and removed T: new feature labels May 28, 2021
@pksunkara pksunkara added this to the 3.0 milestone May 28, 2021
@dimo414
Copy link
Author

dimo414 commented May 30, 2021

@pksunkara I see you added this to the 3.0 milestone, would you welcome a PR changing this to Result<Option<R>, Error>? Or do you have a different fix in mind?

@pksunkara
Copy link
Member

Not sure yet. Because it seems to be tied into other issues for 3.0

@dimo414
Copy link
Author

dimo414 commented May 30, 2021

Ok, happy to help whenever makes sense.

@epage
Copy link
Member

epage commented Jul 22, 2021

A workaround: for this would be to calll is_present

@dimo414
Copy link
Author

dimo414 commented Jul 22, 2021

Sure, but that's not what's documented :) Personally I'd prefer to avoid duplicating the flag name too.

@epage
Copy link
Member

epage commented Jul 22, 2021

Sure, but that's not what's documented

As in there is behavior contrary to documentation or it would help if documentation talked about it?

@dimo414
Copy link
Author

dimo414 commented Jul 22, 2021

The documentation recommends inspecting the ErrorKind, and makes no mention of is_present. IMO the API of value_of_t() should be changed (e.g. to return Result<Option<R>, Error>) but the root of the issue is that the documented way of handling this situation is unclear and cumbersome. If the expectation is actually to use is_present that should be documented. See #2453 if you haven't already, which has a bit more detail.

@epage
Copy link
Member

epage commented Jul 22, 2021

Like I said, I was giving a workaround and not making a determination and what the proper fix is for this.

If we add an Option, I'm mixed on whether it should go on the inside or outside.

@dimo414
Copy link
Author

dimo414 commented Oct 13, 2021

@epage @pksunkara is it still possible to contribute a fix here before 3.0 is cut? It really seems unfortunate that the value_of functionality was redesigned from 2->3 but still carries this limitation.

@pksunkara
Copy link
Member

As you can see from #2683, it is in the plan to rework this a little bit. Just focussing on other things first.

@dimo414
Copy link
Author

dimo414 commented Oct 14, 2021

Ah great thanks! I just wasn't clear whether it was a release blocker. Again, very happy to draft a PR if it would be helpful.

@pksunkara
Copy link
Member

pksunkara commented Oct 14, 2021

It's not a confirmed release blocker. I planned it to be a few months ago but with the other 2 maintainers being active, we need to discuss and see if we want to keep it like that or not.

First step of the discussion is for me to summarize the issues surrounding this in #2683.

@epage
Copy link
Member

epage commented Oct 16, 2021

I'm sorry but this won't be making it in the 3.0 release.

People have been waiting for clap3 for years and people are wanting access to the improvements that are already out there to the point that some have talked about forking it. We need to narrow of focus for this release and come up with a plan for how we want to address these larger issues before making a further commitment on when for something like this.

Short term and long term opportunities:

@epage epage modified the milestones: 3.0, 4.0 Oct 16, 2021
@pksunkara pksunkara added the M-breaking-change Meta: Implementing or merging this will introduce a breaking change. label Oct 16, 2021
@pksunkara pksunkara removed this from the 4.0 milestone Oct 16, 2021
@dimo414
Copy link
Author

dimo414 commented Oct 17, 2021

That's too bad, especially since this functionality was already reimplemented for 3.0 so everyone upgrading will have to swap to the new behavior, only to (presumably) have to do so again at some future date.

@epage epage added A-parsing Area: Parser's logic and needs it changed somehow. and removed C: matches labels Dec 8, 2021
@epage epage added the S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing label Dec 13, 2021
epage added a commit to epage/clap that referenced this issue May 17, 2022
To set the type, we offer
- `ValueParser::<type>` short cuts for natively supported types
- `TypedValueParser` for fn pointers and custom implementations
- `value_parser!(T)` for specialized lookup of an implementation
  (inspired by clap-rs#2298)

The main motivation for `value_parser!` is to help with `clap_derive`s
implementation but it can also be convinient for end-users.

When reading, this replaces nearly all of our current `ArgMatches` getters with:
- `get_one`: like `value_of_t`
- `get_many`: like `values_of_t`

It also adds a `get_raw` that allows accessing the `OsStr`s without
panicing.

The naming is to invoke the idea of a more general container which I
want to move this to.

The return type is a bit complicated so that
- Users choose whether to panic on invalid types so they can do their
  own querying, like `get_raw`
- Users can choose how to handle not present easily (clap-rs#2505)

We had to defer setting the `value_parser` on external subcommands,
for consistency sake, because `Command` requires `PartialEq` and
`ValueParser` does not impl that trait.  It'll have to wait until a
breaking change.

Fixes clap-rs#2505
@dimo414
Copy link
Author

dimo414 commented May 29, 2022

I haven't played with the new parser yet but looking at the PR description it sounds great! Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-enhancement Category: Raise on the bar on expectations M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants