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

Implement TryFrom<String> in EnumString #302

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

wyatt-herkamp
Copy link
Contributor

@wyatt-herkamp wyatt-herkamp commented Oct 9, 2023

Implement TryFrom<String> during EnumString

If no default variant is provided it will just call FromStr other wise it will generate something similar to this

#[allow(clippy::use_self)]
impl ::core::convert::TryFrom<String> for Color {
    type Error = ::strum::ParseError;
    fn try_from(s: String) -> ::core::result::Result<Color, <Self as ::core::convert::TryFrom<String>>::Error> {
        ::core::result::Result::Ok(match s.as_str() {
            "RedRed" => Color::Red,
            "b" => Color::Blue { hue: Default::default() },
            "blue" => Color::Blue { hue: Default::default() },
            "y" => Color::Yellow,
            "yellow" => Color::Yellow,
            _ => return ::core::result::Result::Ok(Color::Green(s.into())), 
        })
    }
}

Could you please add the "hacktoberfest-accepted" label to the PR.

@Peternator7
Copy link
Owner

Hi @wyatt-herkamp, I like the idea of this PR, but I think it'll be a lot clearer if there's some additional refactoring done. Mostly, the MSRV for strum has been increased to 1.60 b/c of syn + quote so we don't need the conditional compilation anymore, and it's probably cleaner to define FromStr in terms of TryFrom rather than the other way around. If we do that, the bodies of TryFrom for &str and String are almost identical.

I see this is a hackathon PR; since it's near the end of Oct, I just wanted to see if it's the type of work you still have time for?

@wyatt-herkamp
Copy link
Contributor Author

Hi @wyatt-herkamp, I like the idea of this PR, but I think it'll be a lot clearer if there's some additional refactoring done. Mostl

I already got the completion
Then I learned they were no longer doing the t-shirts. So it really doesn't matter to me anymore.

I will working on the refactoring next week.

@wyatt-herkamp
Copy link
Contributor Author

For some reason having the nostd tests in the same workspace as the regular tests was causing nostd to use the same macro. So it would add std code. I think this should fix it

@wyatt-herkamp
Copy link
Contributor Author

@Peternator7 so I did not realize cargo weak features were not stabilized until Rust 1.61.

So the options are either.
- Where you have to add the macros depend separately to get the std features.
- Have a second feature inside the strum macro like derive_std
- or bump the minimum rust version to 1.61

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants