-
Notifications
You must be signed in to change notification settings - Fork 161
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
Refactor #133
Refactor #133
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going to switch to Result
style error handling we should try and switch all the panic
s to return Err
instead.
Otherwise, everything looks great.
|
||
for meta in strum_meta { | ||
let meta = match meta { | ||
syn::Meta::NameValue(mv) => mv, | ||
Meta::NameValue(mv) => mv, | ||
_ => panic!("strum on types only supports key-values"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we return an Err here was well?
_ => panic!("strum on types only supports key-values"), | ||
}; | ||
|
||
if meta.path.is_ident("serialize_all") { | ||
let style = match meta.lit { | ||
syn::Lit::Str(s) => s.value(), | ||
Lit::Str(s) => s.value(), | ||
_ => panic!("expected string value for 'serialize_all'"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well.
let name = &ast.ident; | ||
let vis = &ast.vis; | ||
|
||
let variants = match ast.data { | ||
syn::Data::Enum(ref v) => &v.variants, | ||
Data::Enum(ref v) => &v.variants, | ||
_ => panic!("EnumDiscriminants only works on Enums"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this return a result?
@@ -24,17 +25,19 @@ pub fn enum_iter_inner(ast: &syn::DeriveInput) -> TokenStream { | |||
}; | |||
|
|||
let variants = match ast.data { | |||
syn::Data::Enum(ref v) => &v.variants, | |||
Data::Enum(ref v) => &v.variants, | |||
_ => panic!("EnumIter only works on Enums"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can use Span.call_site()
as the error position here.
Yes, I'm planning to convert more panics to |
Change is merged. I'm going to hold off publishing a new version until the rest of your changes are in. |
No description provided.