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 support for #derive[FromStr] for enums #176

Merged
merged 1 commit into from
Feb 8, 2022

Conversation

aj-bagwell
Copy link
Contributor

This allows deriving FromStr for enums with no fields in the obvious way

fixes #59

type Err = #err_name;
#[inline]
fn from_str(src: &str) -> ::core::result::Result<Self, Self::Err> {
Ok(match src.to_lowercase().as_str() {
Copy link
Contributor Author

@aj-bagwell aj-bagwell Dec 3, 2021

Choose a reason for hiding this comment

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

I considered adding .replace(&[' ', '_', '-'], "") here so that it would mach against kebab-case and SNAKE_CASE too, but I'm not sure how lenient it should be, I am already on the fence about the case insensitive matching.

quote! {

#[derive(Debug, Clone, PartialEq, Eq)]
#visibility struct #err_name;
Copy link
Contributor Author

@aj-bagwell aj-bagwell Dec 3, 2021

Choose a reason for hiding this comment

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

I'm not sure I like creating a type out of nothing here, but I did not like the other options better:

  • could use &'static str but that does not impelement std::error::Error which makes it annoying for combining with other errors
  • could have a derive_more::ParseError but that complicates the dependencies as I don't think you can have code and procmarcos in the same dependency

@aj-bagwell aj-bagwell force-pushed the enum_from_str branch 2 times, most recently from 4c17d26 to ec7c52b Compare December 3, 2021 11:17
@aj-bagwell
Copy link
Contributor Author

aj-bagwell commented Dec 3, 2021

Forced pushed changes that hopefully fix the failing tests

@@ -51,7 +51,7 @@ deref_mut = []
display = ["syn/extra-traits"]
error = ["syn/extra-traits"]
from = ["syn/extra-traits"]
from_str = []
from_str = ["convert_case"]
Copy link
Owner

Choose a reason for hiding this comment

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

This should fix the current compilation issue in the feature tests.

Suggested change
from_str = ["convert_case"]
from_str = ["convert_case", "syn/extra-traits"]

Copy link
Contributor Author

@aj-bagwell aj-bagwell Dec 3, 2021

Choose a reason for hiding this comment

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

Thanks, I just forced pushed a change that bumps syn to 1.0.81 (which is the version in the lock file) and changed the code to use .is_empty() instead which means it does not need the extra traits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bah looks like I also need to ditch the x == from_str(x.to_string()) test as I can't #derive[Display] if the display feature is not set.

@aj-bagwell aj-bagwell force-pushed the enum_from_str branch 3 times, most recently from 8607fa0 to c4190a8 Compare December 3, 2021 14:33
@nirvana-msu
Copy link

Is there anything blocking this PR from progressing?

@aj-bagwell
Copy link
Contributor Author

The test are all now passing and I'm happy with my commits as they stand, @JelteF let me know if there are any more changes that you want me to make.

Comment on lines +93 to +98
for variant in variants {
let variant_str = variant.to_string();
cases.push(quote! {
#canonical if(src == #variant_str) => #input_type::#variant,
})
}
Copy link
Owner

Choose a reason for hiding this comment

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

this edge case is missing a test

JelteF
JelteF previously approved these changes Feb 8, 2022
Copy link
Owner

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

I think this is good except for a missing test for the smart case sensitive behaviour.

This allows deriving FromStr for enums with no fields in the obvious way

fixes JelteF#59
@aj-bagwell
Copy link
Contributor Author

I have added enum_test_case_sensitive to check if falls back to case sentive matching when it matters and rebased onto master to bring it up to date

@JelteF JelteF merged commit e32f499 into JelteF:master Feb 8, 2022
@aj-bagwell aj-bagwell deleted the enum_from_str branch February 8, 2022 12:13
@JelteF
Copy link
Owner

JelteF commented Feb 8, 2022

Thanks a lot for your work on this!

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 FromStr for Enum
3 participants