-
Notifications
You must be signed in to change notification settings - Fork 248
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
Migrate custom error trait impls to thiserror
#1856
base: master
Are you sure you want to change the base?
Conversation
core/src/events.rs
Outdated
@@ -376,7 +376,8 @@ impl<T: Config> EventDetails<T> { | |||
.map(|f| scale_decode::Field::new(f.ty.id, f.name.as_deref())); | |||
|
|||
let decoded = | |||
scale_value::scale::decode_as_fields(bytes, &mut fields, self.metadata.types())?; | |||
scale_value::scale::decode_as_fields(bytes, &mut fields, self.metadata.types()) | |||
.map_err(Into::<scale_decode::Error>::into)?; |
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.
I'm curious what has changed that makes these new .map_err
s necessary?
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.
decode_as_fields returns DecodeError
and although scale_decode::Error
implements From<DecodeError>
compiler needs an explicit cast
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.
I don't get why that would be the case though offhand, when it wasn't before? Also, out of interest, can you use Into::into
or do you need the explicit type there too?
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.
No idea either.
Fails to infer the type without explicit annotation
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.
I had a look and there was a missing From
impl for scale_decode::visitor::DecodeError
(confusingly similar name!); adding that let the Into's go away :)
Co-authored-by: Niklas Adolfsson <[email protected]>
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.
Looks good to me.
Would be good if you could double-check whether we should use #[error(transparent)]
from every wrapped error type to avoid duplicate stuff in the display impl.
Such Decode error: Decode error <some message>
but I'm not sure I could be wrong :P
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.
Nice!
Description
Migrates Error trait impls to
thiserror
as it now supports nostd