-
Notifications
You must be signed in to change notification settings - Fork 918
units: Improve the decoders #5027
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
Conversation
|
cc @nyonson, I know you are watching but just to be sure. |
c0448f8 to
672c08a
Compare
672c08a to
96908b3
Compare
| } | ||
|
|
||
| #[cfg(feature = "encoding")] | ||
| impl From<encoding::UnexpectedEofError> for AmountDecoderError { |
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.
New pattern for me here so might be missing something, but would this From impl still be useful if updates to the new constructor?
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.
The reason for removing this is that if we keep it we have to commit to it, so we cannot stop using the UnexpectedEofError. As much as this is unlikely I'm trying to be as conservative as possible and since the from impl is only used internally there is no need for it other than ergonomics (in internal code). Also downstream should never be constructing AmountDecoderError.
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.
Got it, thanks for the explanation. Wasn't obvious to me how the From was part of the exported API.
a2cac48 to
c3c49a9
Compare
Introduces new policy on how we implement decoder error types. - All internals are hidden - No `From<OtherError>` impls - Private constructors - Inner enum is private (does not need non_exhaustive).
If two encoders return the same error it is not currently possible to compose them with our decoders. Having a new type instead of returning the `UnexpectedEofError` makes this possible (assuming one doesn't want to compose two decoders of the same type).
So the decoders can be used by downstream add constructors for them all. Also implement `Default` for the decoder types.
c3c49a9 to
d340c4d
Compare
As we do for other `signed` and `unsigned` types; re-export the `AmountDecoder` from the `amount` module.
|
You need another API run after the last commit. |
|
But utACK 6175586 |
apoelstra
left a comment
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.
ACK b8af0d7; successfully ran local tests
Improve all the decoders and decoder error types in
units. Based on recent discussion.This PR introduces policy on how we implement decoder error types (patch 1 and 2).
LFG, decoders done by Nashville!