refactor: ownership/from traits in e2store#1730
Conversation
looks good
morph-dev
left a comment
morph-dev
left a comment
There was a problem hiding this comment.
I found several places where we use impl TryFrom<& X > for Entry, but conversion never fails. I know that this is similar to original implementation (using TryInto), and not something that you introduced.
I would say we should use impl From<& X > for Entry whenever we can.
Looking into other types and their conversion into Entry, it seems that error can happen only during snappy encoding. But since we are always writing to Vec, I think even that can't fail, and I wouldn't be against just using .expect in that case. But I'm not going to insist on this as it's cleaner to return Error if it can fail, but I would maybe change error type to std::io::Error in that case (instead of generic anyhow::Error).
| } | ||
|
|
||
| impl TryInto<Entry> for E2HSBlockIndexEntry { | ||
| impl TryFrom<&E2HSBlockIndexEntry> for Entry { |
There was a problem hiding this comment.
If I'm not mistaken, this conversion can't fail. So I would just implement From<&E2HSBlockIndexEntry> instead.
| } | ||
|
|
||
| impl TryInto<Entry> for SlotIndexBlockEntry { | ||
| impl TryFrom<&SlotIndexBlockEntry> for Entry { |
There was a problem hiding this comment.
Also here, maybe just do impl From<&SlotIndexBlockEntry>
| } | ||
|
|
||
| impl TryInto<Entry> for SlotIndexStateEntry { | ||
| impl TryFrom<&SlotIndexStateEntry> for Entry { |
| } | ||
|
|
||
| impl TryInto<Entry> for TotalDifficultyEntry { | ||
| impl TryFrom<&TotalDifficultyEntry> for Entry { |
| } | ||
|
|
||
| impl TryInto<Entry> for AccumulatorEntry { | ||
| impl TryFrom<&AccumulatorEntry> for Entry { |
| } | ||
|
|
||
| impl TryInto<Entry> for Era1BlockIndexEntry { | ||
| impl TryFrom<&Era1BlockIndexEntry> for Entry { |
3 participants
What was wrong?
Addressed some remaining comments from #1727 related to ownership patterns we use in
e2storeHow was it fixed?
To-Do