value_of_t() is difficult to use with optional arguments

[epage]

Issue by dimo414
Friday May 28, 2021 at 07:23 GMT
Originally opened as clap-rs/clap#2505

---

Please complete the following tasks

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

Describe your use case

See clap-rs/clap#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

[epage]

Comment by dimo414
Sunday May 30, 2021 at 07:33 GMT

---

@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?

[epage]

Comment by pksunkara
Sunday May 30, 2021 at 08:27 GMT

---

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

[epage]

Comment by dimo414
Sunday May 30, 2021 at 09:02 GMT

---

Ok, happy to help whenever makes sense.

[epage]

Comment by epage
Thursday Jul 22, 2021 at 14:24 GMT

---

A workaround: for this would be to calll is_present

[epage]

Comment by dimo414
Thursday Jul 22, 2021 at 17:47 GMT

---

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

[epage]

Comment by epage
Thursday Jul 22, 2021 at 17:51 GMT

---

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?

[epage]

Comment by dimo414
Thursday Jul 22, 2021 at 19:15 GMT

---

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 clap-rs/clap#2453 if you haven't already, which has a bit more detail.

[epage]

Comment by epage
Thursday Jul 22, 2021 at 19:19 GMT

---

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.

[epage]

Comment by dimo414
Wednesday Oct 13, 2021 at 17:47 GMT

---

@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.

[epage]

Comment by pksunkara
Wednesday Oct 13, 2021 at 17:50 GMT

---

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

[epage]

Comment by dimo414
Thursday Oct 14, 2021 at 01:04 GMT

---

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.

[epage]

Comment by pksunkara
Thursday Oct 14, 2021 at 01:07 GMT

---

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]

Comment by epage
Saturday Oct 16, 2021 at 18:20 GMT

---

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:

• While I know its not for everyone, clap_derive (fork of structopt) takes care of problems like this.
• I have floated the idea of baking the expected types into ArgMatches, which would remove the need for these functions and would instead behave like the regular ones
• We flesh out and implement @pksunkara idea in #2683

[epage]

Comment by dimo414
Sunday Oct 17, 2021 at 16:58 GMT

---

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.