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

Add TryFrom<&str> to EnumString #186

Merged
merged 1 commit into from
Oct 26, 2021
Merged

Conversation

dspicher
Copy link
Contributor

@dspicher dspicher commented Oct 19, 2021

The implementation simply delegates to std::str::FromStr.

To maintain compatibility with Rust 1.32, a new dependency
is introduced which allows us to emit the TryFrom<&str>
implementation only for Rust 1.34 and above when
std::convert::TryFrom was stabilized.

Closes #107.

@dspicher dspicher force-pushed the master branch 2 times, most recently from 81781f2 to 55e00c9 Compare October 19, 2021 18:38
Copy link
Owner

@Peternator7 Peternator7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addition of rustversion crate makes sense. Could you update the docs as well to say this is implemented when rustc >= 1.34?

);

Ok(std::iter::FromIterator::from_iter(
vec![from_str, try_from_str].into_iter(),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do this?

Ok(quote! {
    #from_str
    #try_from_str
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of course, much nicer!

@@ -17,39 +17,46 @@ enum Color {
Black,
}

macro_rules! assert_from_str {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably needs to account for rustc version as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I added rust version-dependent macros.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got rid of the macros altogether and used normal functions with appropriate trait bounds.

@dspicher
Copy link
Contributor Author

Thanks for the fast review @Peternator7, I just force-pushed some fixes.

) -> TokenStream {
quote! {
#[allow(clippy::use_self)]
impl #impl_generics ::std::convert::TryFrom<&str> for #name #ty_generics #where_clause {
Copy link

@toxeus toxeus Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe using AsRef<str> would be even more useful?

impl<T: AsRef<str>> #impl_generics ::std::convert::TryFrom<T> for #name #ty_generics #where_clause {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This runs into the following interesting compiler bug:
rust-lang/rust#50133

See e.g. here for our very specific case: rust-lang/rust#50133 (comment)

Leaving it out for now, since the workaround looks annoying to use.

The implementation simply delegates to `std::str::FromStr`.

To maintain compatibility with Rust 1.32, a new dependency
is introduced which allows us to emit the `TryFrom<&str>`
implementation only for Rust 1.34 and above when
`std::convert::TryFrom` was stabilized.
@dspicher
Copy link
Contributor Author

Addition of rustversion crate makes sense. Could you update the docs as well to say this is implemented when rustc >= 1.34?

Sorry I had missed this, updated the docs now.

@Peternator7
Copy link
Owner

This looks good to me. Anything you wanted to add or is this ready to merge?

@dspicher
Copy link
Contributor Author

This looks good to me. Anything you wanted to add or is this ready to merge?

No, good to go from my side.

@Peternator7 Peternator7 merged commit 7b68e4d into Peternator7:master Oct 26, 2021
@Peternator7
Copy link
Owner

Released as part of 0.23. Thanks for the PR. https://crates.io/crates/strum

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.

Implement std::convert::TryFrom<&str> for EnumString
3 participants