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

Make FromStr::Err not need to be a Send+Sync+'static #5149

Closed
wants to merge 2 commits into from

Conversation

chabapok
Copy link

As far as I understand, standard rust library does not require error::Error to be a Send+Sync+'static, and (for example) structopt does not. But clap require. Thus, the library turns out to be incompatible with most parsing libraries that return Box<dyn Error> as an error.

This commit proposes to relax this requirement. This makes the library more backward-compatible with structopt. This will allow errors to be thrown from libraries that return errors, that not Send or Sync, for example ssсanf:

impl FromStr for MyConfigElement {
    type Err = sscanf::Error; // <<-- sscanf library return Box<dyn Error>, not Box<dyn Error+Send+Sync>
    // so sscanf::Error don't works with clap, but it's works in old 'structopt' crate

    fn from_str(s: &str) -> Result<Self, Self::Err> {

        #[derive(sscanf::FromScanf)]
        #[sscanf(format = "{foo}")]
        struct ParseHelper {
            foo: u32,
        }

	let result = sscanf::sscanf!(s, "{ParseHelper}")?;
	Ok(MyConfigElement::new(result.foo))
    }
}

A test+example of using such errors has also been added.

I know that a year ago there was a discussion about how the return Ok() result could also be !send+!sync+!clone (#3833), and I tried to do that - but it turned out that it required a lot of work.

@chabapok chabapok force-pushed the master branch 2 times, most recently from 38db44b to 3fde2a5 Compare September 30, 2023 12:36
@epage
Copy link
Member

epage commented Sep 30, 2023

Per the contributing guide, changes like this need to be discussed in an issue first.

Considering this is a breaking change, I'm going to go ahead and close this. Feel free to make an issue and if we agree on the direction, we can then talk about strategies for going forward (ie unstable feature flag)

@epage epage closed this Sep 30, 2023
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.

2 participants