Cleanup error type mapping in serde_snapshot #10580
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10580 +/- ##
=======================================
Coverage 81.7% 81.8%
=======================================
Files 301 301
Lines 70634 70614 -20
=======================================
Hits 57775 57775
+ Misses 12859 12839 -20 |
|
Thanks for working on general clean-ups! |
| .map_err(accountsdb_to_io_error); | ||
| if e.is_err() { | ||
| info!("{:?}", e); | ||
| continue; |
There was a problem hiding this comment.
This ill-taste continue was a work-around introduced at: https://github.com/solana-labs/solana/pull/7281/files#diff-2099c5256db4eb5975c8834af38f6456R503
With good confidence, we no longer need this. This isn't no longer warning. It's safe to cause error and abort now as this is really bad condition now. Could you remove continue and adjust the new code so that the work-around in that dated pr will be effectively reverted?
There was a problem hiding this comment.
(For the record: this workaround got deprecated at #8339)
| serialized_len, | ||
| deserialize_from(&mut stream).map_err(accountsdb_to_io_error)?, | ||
| ); | ||
| let serialized_len = min(serialized_len, deserialize_from(&mut stream)?); |
There was a problem hiding this comment.
Yay! this reads a lot better. :)
|
@svenski123 Admitted, error handling is rough areas in our codebase. Thanks for polishing! You might find these interesting as a further direction towards better error handling:
|
|
Regarding error types, I think something like https://docs.rs/anyhow/1.0.31/anyhow/ would make sense in a number of places. |
Problem
Excessive explicit error type mapping is unclear
Summary of Changes
Rework error types to allow use of '?' operator without explicit map_err() calls