Skip to content

Add TryFrom<&str> to EnumString#186

Merged
Peternator7 merged 1 commit intoPeternator7:masterfrom
dspicher:master
Oct 26, 2021
Merged

Add TryFrom<&str> to EnumString#186
Peternator7 merged 1 commit intoPeternator7:masterfrom
dspicher:master

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!

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