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
Improve leaf errors #2530
Improve leaf errors #2530
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.
ACK da98a84 I kinda think invalid()
should have a better name but I am not sure what
I'm guessing a better naming convention will fall out once I've done this a few more times. I"m was leaning towards making the names a bit longer than usual but @Kixunil suggested shortening one recently. Not totally sure either way. Shall we merge and iterate? |
Pull Request Test Coverage Report for Build 8242086240Details
💛 - Coveralls |
|
||
impl TryFromError { | ||
/// Returns the invalid non-witness version integer. | ||
pub fn invalid(&self) -> u8 { self.invalid } |
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.
Yeah, this is too confusing I think. I'd prefer provided_version
or something along those lines.
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.
Used invalid_version
.
Leaf error types should typically have private fields, provide accessor functions, and not use `non_exhaustive`.
Leaf error types should typically have private fields, provide accessor functions, and not use `non_exhaustive`.
da98a84
to
f8de795
Compare
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. Waiting for the resolution on the necessity of last commit.
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 f8de795
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 f8de795
In light of recent discussion go over the codebase and look for some places that the leaf errors are wrong. Does not do the whole code base, excludes
p2p
and a couple of other places.