-
Notifications
You must be signed in to change notification settings - Fork 240
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
Remove derive_more
#1600
Remove derive_more
#1600
Conversation
53df0e8
to
885bbb3
Compare
Feel free to merge the two sub-PRs into this one; IMO it would be easier to see it all in one place, and you'll have to merge the others into here before we can properly review this one anyway! I think I'd only normally stick to separate PRs when it's actually two distinct things that can both be merged to master independently :) |
d8eb474
to
2392b58
Compare
I initially made separate pr's to make reviewing easier, but good to know, i won't be splitting mr's in the future if they are changing the same thing |
core/src/error.rs
Outdated
Error::Decode(value.into()) | ||
} | ||
} | ||
convert_error!(ExtrinsicParamsError as Error::ExtrinsicParams); |
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.
What do you think about:
impl_from!(ExtrinsicParamsError => Error::ExtrinsicParams);
I find "as" slightly misleading because in my head as == typecast
such as "usize::MAX as u64"....
Ideally we could find a similar crate as derive_more or some other crate for error handling where it supports opting out from std::error::Error which is the reason why we are not using thiserror in subxt but this should work as work around for now.
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.
changed convert_from
to impl_from
with the as
-> =>
swap
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.
LGTM! I do like the suggestion from Niklas to have a impl_from
instead of convert_err
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, LGTM! (I'd also be up for that impl_from
and =>
thing but easy either way :)
Co-authored-by: Niklas Adolfsson <[email protected]>
3200244
to
e6b6394
Compare
Description
From
fromderive_more
derive_more
from dependenciesQuestions:
Given that we used
derive_more
for errors, maybe we can use SNAFU for errors with no-std constraint given thatthiserror
doesn't support no-stdsee #1503