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

feat: Allow referencing TypedValueParser inValueParserFactory associated type #5066

Merged
merged 1 commit into from Aug 8, 2023

Conversation

9999years
Copy link
Contributor

@9999years 9999years commented Aug 3, 2023

When searching for relevant discussions and issues I happened upon #4362, which suggests using TypedValueParser::try_map. Unfortunately, this is not suitable for ValueParserFactory implementations that automatically register a TypedValueParser for a given type, as the TryMapValueParser type returned from try_map is not public.

Partial fix for #5065

Partial fix for clap-rs#5065, allows using TypedValueParser::try_map to
implement a ValueParserFactory.
@epage
Copy link
Member

epage commented Aug 4, 2023

Let's hold off on solutions until we've figured out the problem and how we should be solving it.

@epage epage closed this Aug 4, 2023
@epage epage reopened this Aug 8, 2023
@epage epage changed the title Export builder::TryMapValueParser feat: Allow referencing TypedValueParser inValueParserFactory associated type Aug 8, 2023
@epage
Copy link
Member

epage commented Aug 8, 2023

I've updated the PR to reflect the clearer explanation from #5065.

Technically, this isn't needed. You can create your own type and then call InnerValueParser::new().try_map(...).parse_ref inside of your value parser but exposing this makes things a bit easier.

And technically, this fix is limited because it won't help when the function passed to try_map is unnameable. Associated type impl traits is really what we need. However, there is unlikely to be a case that needs unnameable types.

@epage epage merged commit 3156e1a into clap-rs:master Aug 8, 2023
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants