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

TypedValueParser cannot parse/validate an OsStr with full error context #4362

Closed
2 tasks done
kpreid opened this issue Oct 9, 2022 · 6 comments · Fixed by #4365
Closed
2 tasks done

TypedValueParser cannot parse/validate an OsStr with full error context #4362

kpreid opened this issue Oct 9, 2022 · 6 comments · Fixed by #4365
Labels
C-bug Category: Updating dependencies

Comments

@kpreid
Copy link
Contributor

kpreid commented Oct 9, 2022

Please complete the following tasks

Clap Version

4.0.11 (and also 3.2)

Describe your use case

There seems to be no way to simultaneously:

  • run custom parsing or validation code on an argument (via value_parser = ...),
  • accept OsStr input (non-UTF-8) rather than str, and
  • produce errors with context information identifying the erroneous value etc.

There is an implementation of TypedValueParser for functions which adds context for the error but it only accepts &str input, and if one is writing one's own impl TypedValueParser for … then it is not possible to access the Error operations to add context. The documentation recommends Command::error(), but that takes a mutable Command which is not available from within a TypedValueParser.

Describe the solution you'd like

It should be possible to construct a clap::Error, while implementing a parser, that has all the usual context.

Alternatives, if applicable

A second implementation for functions that take &OsStr would work, but does not seem an ideal solution to me since it doesn't allow composing TypedValueParsers.

Additional Context

In previous discussion, epage wrote:

But we should also support people writing their own value parsers and this is a bug hole for it. Can you create an issue?

No one filed that issue yet, so I am now. (Another discussion post.)

@kpreid kpreid added the C-enhancement Category: Raise on the bar on expectations label Oct 9, 2022
@kpreid
Copy link
Contributor Author

kpreid commented Oct 9, 2022

(I think this should be labeled a bug rather than an enhancement, since it's functionality that existed in clap 3, but the "enhancement" issue template was more fitting for what there was to say about it.)

@epage epage added C-bug Category: Updating dependencies and removed C-enhancement Category: Raise on the bar on expectations labels Oct 10, 2022
@epage
Copy link
Member

epage commented Oct 10, 2022

For anyone else reading this, I would recommend double checking if you need to implement TypedValueParser. Too frequently people immediately jump to implementing it when that is the harder and more verbose route

Current alternatives:

  • Any Fn(&str) -> Result<T, E> already implements it
  • value_parser! can adapt types that implement FromStr, From<&str>, From<String>, From<&OsStr> and From<OsString>
  • You can take existing TypedValueParsers and call TypedValueParser::map on them

@epage
Copy link
Member

epage commented Oct 10, 2022

@kpreid could you expand on your use case? I'm curious in what situations someone needs to parse OsStr or OsString types. That'll also help in understanding if we should also look into supporting TryFrom, TypedValueParser::try_map, or a host of additional solutions on top of exposing more error context.

@epage
Copy link
Member

epage commented Oct 10, 2022

For this issue, there are two core problems

  • We don't expose a way to set Error::context
  • We don't expose a way to enable coloring and other similar features

@kpreid
Copy link
Contributor Author

kpreid commented Oct 10, 2022

could you expand on your use case?

In the particular case, I want to validate that --output <FILE> has a recognizable filename extension. I could do this validation on the final struct post-clap-parsing of course, but then I still don't have the ability to provide all the nice formatted error context, at least without doing a lot of digging to get an &mut Command (which, besides being inelegant, also doesn't fit well the clap derive system which allows the code that runs after clap parsing to not care about anything but the struct fields).

A more elaborate case of actual parsing involving OsStr input might be, say, expecting a glob pattern or a numeric-sequence pattern.

That'll also help in understanding if we should also look into supporting…

Prior to clap 3.2 I was using the validator_os option. So, TypedValueParser::try_map would be perhaps a good option, if it then supports returning an error that will be given context (as opposed to the map function having to assemble the desired context itself). While setting arbitrary context would be sufficient, it would be preferable to be able to express “this is a validation error; please insert all the context you would for that” without having to duplicate that fragment of clap's internal logic. That way, good and consistent messages appear by default.

epage added a commit to epage/clap that referenced this issue Oct 10, 2022
epage added a commit to epage/clap that referenced this issue Oct 10, 2022
This is a major building block to avoid needing to implement
`TypedValueParser`

Inspired by clap-rs#4362
@kpreid
Copy link
Contributor Author

kpreid commented Oct 11, 2022

I just confirmed that release 4.0.12 solves the problem I had. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Updating dependencies
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants