Use expect instead of unwrap for calls to consensus_encode#719
Use expect instead of unwrap for calls to consensus_encode#719RCasatta merged 4 commits intorust-bitcoin:masterfrom
Conversation
In the name of uniformity use the same error message as argument to `expect` througout the codebase. Use "engines don't error" instead of "engines don't err".
Calls to `unwrap` outside of tests are typically unfavourable. Hash engines do not error when calling `consensus_encode`. Instead of the current usage of `unwrap` we can use `expect` with a descriptive string as is done in other parts of the codebase.
Calls to `unwrap` outside of tests are typically unfavourable. Sink writers do not error. We can use `expect` with a descriptive message string to indicate this.
Calls to `unwrap` outside of tests are typically unfavourable. In memory writers (`Vec`) do not error. We can use `expect` with a descriptive message string to indicate this.
|
I think we need this change; the only question is how to write the message in So I think expect messages should be written in form saying what happened and not what cant't happen: |
|
@dr-orlovsky it's not as straightforward. I commonly use the message instead of a comment explaining why it's OK to panic be cause I expect it to never happen. (Although once I did actually experience a funny message "std command API is retarded".) If the message is not explaining why it shouldn't happen then there needs to be a comment. |
|
I used to write the |
There was a problem hiding this comment.
LGTM, but please avoid spending too much time on this kind of thing. These types of trivial changes in unreachable code, or other trivial code motion that doesn't change user-experience and doesn't change feature sets are largely responsible for the standstill in major improvements that has plagued the Bitcoin Core project for years - lets not extend it to rust-bitcoin please.
Trivial fixes are great when they're treated as trivial and merged quickly - when it ends up with reviewers trying to debate nuance of the Most Correct version of a trivial fix it can suck forward progress out of a project.
Point taken, I'll try to keep this in mind.
I have tendency to do a bunch of theses trivial patches especially when first entering a project, to get a feel for the project and to set a few rules down for myself going forward. Perhaps I'll preface the description of such PRs with "Please ack and merge or nack and close, this is trivial work not worth debating"? Please do keep coming at me with knowledge like this @TheBlueMatt, the better I can understand how to help rust-bitcoin progress the better for the project :) Cheers, |
That's totally fine! The second half of my statement was less directed at you and more other reviewers! :) |
RCasatta
left a comment
There was a problem hiding this comment.
ACK e7b84e2
I wanted to merge, but using the github-merge.py tool resut "Top commit has no ACKs!".
This clearly has 2 ACKs, because @TheBlueMatt approved and @Kixunil acked the bottom commit instead of the top one, but I am not sure we want to enforce the two acks to be visible in the merge commit so I momentarily suspended the merge
|
Sorry for confusion. ACK e7b84e2 |
Calls to
unwrapoutside of tests are generally unfavourable. We currently callunwrapin a bunch of places on calls toconsensus_encodewhen passing writers that do not fail.Remove
unwrapcalls on all calls toconsensus_encodethat pass a writer argument for which write functions do not fail. Useexpectwith a descriptive string instead.Fixes: #714