feat: make codec generic over FileClient
#12521
Conversation
TropicalDog17
requested review from
onbjerg,
rkrasiuk,
emhane and
mattsse
as code owners
| @@ -447,15 +464,16 @@ impl ChunkedFileReader { | |||
| } | |||
|
|
|||
| /// Read next chunk from file. Returns [`FileClient`] containing decoded chunk. | |||
| pub async fn next_chunk<T>(&mut self) -> Result<Option<T>, T::Error> | |||
| pub async fn next_chunk<T, F>(&mut self, codec: T) -> Result<Option<F>, F::Error> | |||
There was a problem hiding this comment.
this arg isn't needed, instead use self.codec
There was a problem hiding this comment.
but codec field only in FileClient, not ChunkedFileReader @emhane
There was a problem hiding this comment.
true, my bad, I pushed a commit since couldn't add a GitHub suggestion on the code i was thinking about
There was a problem hiding this comment.
thanks for your help!
| @@ -49,6 +47,10 @@ pub struct FileClient { | |||
|
|
|||
| /// The buffered bodies retrieved when fetching new headers. | |||
| bodies: HashMap<BlockHash, BlockBody>, | |||
|
|
|||
| /// The codec used to decode the file. | |||
| #[allow(dead_code)] | |||
There was a problem hiding this comment.
I think this attribute can be removed now
|
|
||
| /// The codec used to decode the file. | ||
| codec: T, |
There was a problem hiding this comment.
right so we can remove this field completely...and the generic on file client...would you be ok with doing this last thing or should I?
There was a problem hiding this comment.
Yeah I will try to do it
There was a problem hiding this comment.
I'm not really sure how to remove the generic? Isn't it the part of this pull request, or should I use phantom field for the type T?
There was a problem hiding this comment.
the pr makes it apparent that the generic isn't needed: the codec is passed as arg instead, there is no need to save it in the FileClient type (the unused code warning is rust compilers way of hinting about this!).
the current design of how FileClient and ChunkedFileReader are connected, isn't great rn #7650, apologies for the confusion!
Closes #12475