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

#[clap(parse(try_from_str))] doesn't use try_from(&str) #2437

Closed
Tracked by #2683
TilCreator opened this issue Apr 9, 2021 · 7 comments
Closed
Tracked by #2683

#[clap(parse(try_from_str))] doesn't use try_from(&str) #2437

TilCreator opened this issue Apr 9, 2021 · 7 comments
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies S-wont-fix Status: Closed as there is no plan to fix this

Comments

@TilCreator
Copy link

TilCreator commented Apr 9, 2021

Please complete the following tasks

I have searched the discussions
I have searched the existing issues

Rust Version

rustc 1.50.0 (cb75ad5db 2021-02-10)

Clap Version

3.0.0-beta.2

Minimal reproducible code

use reqwest::Url;
use scraper::Selector;

/*
Available functions:
reqwest::Url::try_from(s: &'a str) -> Result<Self, Self::Error>
reqwest::Url::from_str(input: &str) -> Result<Url, crate::ParseError>
scraper::Selector::try_from(s: &'i str) -> Result<Self, Self::Error>
*/

#[derive(Clap, Debug)]
#[clap(name = env!("CARGO_PKG_NAME"))]
struct Args {
    // There is no problem with parsing url. it's just a working example
    #[clap(
        takes_value = true,
        parse(try_from_str)
    )]
    url: Url,
    #[clap(
        takes_value = true,
        parse(try_from_str),
    )]
    selector: Selector,
}

fn main() {
    let args = Args::parse().unwrap();
}

Steps to reproduce the bug with the above code

cargo run -- https://exameple.com body

Actual Behaviour

It does not compile because #[clap(parse(try_from_str))] doesn't find Selector::try_from(&str).

Expected Behaviour

It compiles and finds the try_from(&str) of Selector.

Additional Context

From the name try_from_str I would expect Clap to use try_from(&str) -> Result<_> rather than (/both) from(&str) -> Result<_>.
#1661

Debug Output

[dependencies]
reqwest = "0.11.2"
scraper = "0.12.0"
clap = "3.0.0-beta.2"
@TilCreator TilCreator added the C-bug Category: Updating dependencies label Apr 9, 2021
@TilCreator TilCreator changed the title #[clap(parse(try_from_str)] doesn't use try_from(&str) #[clap(parse(try_from_str))] doesn't use try_from(&str) Apr 9, 2021
@pksunkara pksunkara added this to the 3.0 milestone Apr 9, 2021
@pksunkara pksunkara added the A-derive Area: #[derive]` macro API label Oct 16, 2021
@epage
Copy link
Member

epage commented Oct 16, 2021

It looks like this also exists in structopt.

The challenge is naming. For our From<T> versions, we need to differentiate T, which leads to from_str for From<&str> and from_os_str for From<&OsStr>. That then conflicts with using from_str for FromStr.

Granted, if we did auto-detection like #2298, this problem goes away except when the user wants to override for a specific case.

@epage
Copy link
Member

epage commented Oct 16, 2021

Forgot to add, from observation, it looks like the naming scheme has less to do with what trait is the default but describes what the function signature is when providing a version of it.

From that perspective, I'd be tempted to Close this as Won't Fix.

@epage
Copy link
Member

epage commented Oct 16, 2021

I've not confirmed it but I believe the user workaround would be parse(try_from_str = Url::try_from)

@epage epage removed this from the 3.0 milestone Oct 16, 2021
@TilCreator
Copy link
Author

TilCreator commented Oct 16, 2021

The workaround works for most cases, but some use weird types where a wrapper is still necessary. Here are examples for both:
My original example with the workaround:

use clap::Clap;
use reqwest::Url;
use scraper::Selector;
use std::convert::TryFrom;

/*
Available functions:
reqwest::Url::try_from(s: &'a str) -> Result<Self, Self::Error>
reqwest::Url::from_str(input: &str) -> Result<Url, crate::ParseError>
scraper::Selector::try_from(s: &'i str) -> Result<Self, Self::Error>
*/

#[derive(Clap, Debug)]
#[clap(name = env!("CARGO_PKG_NAME"))]
struct Args {
    // There is no problem with parsing url. it's just a working example (also from `try_from`)
    #[clap(
        takes_value = true,
        parse(try_from_str = Url::try_from),
    )]
    url: Url,
    #[clap( // Fails with many compile errors
        takes_value = true,
        parse(try_from_str = Selector::try_from),
    )]
    selector: Selector,
}

fn main() {
    let args = Args::parse();

    println!("{:#?}", args);
}

That sadly doesn't fix it completely, because the Selector type doesn't play nice.
For such cases a wrapper case can be used, here an example from one of my projects for that:
Wrapper definition
Wrapper usage

If you don't have to add anything this can be closed, thx

@pksunkara
Copy link
Member

Let's keep this open since I have the impression of tackling this as part of #2683.

@epage
Copy link
Member

epage commented Oct 16, 2021

    url: Url,
    #[clap( // Fails with many compile errors
        takes_value = true,
        parse(try_from_str = Selector::try_from),
    )]
    selector: Selector,*/

The errors have less to do with TryFrom vs FromStr and more to do with the fact that scrapers error type does not impl Error, making it incompatible with most of the Rust ecosystem.

Technically, you do not need a full wrapper type to resolve this. You can do it just by defining a function and passing that in instead.

@epage
Copy link
Member

epage commented Dec 13, 2021

Since the names are intended to convey the signature, not the trait, this is working as intended and I'm going to go ahead and close this.

@pksunkara if there are parts of this you want to resolve in #2683, please comment in that issue with it.

If there are any concerns about this, let us know!

@epage epage closed this as completed Dec 13, 2021
@epage epage added the S-wont-fix Status: Closed as there is no plan to fix this label Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies S-wont-fix Status: Closed as there is no plan to fix this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants