added support for custom configs on streaming structs#2
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds support for custom encoding configurations to the streaming encoder/decoder types, while also gating a couple compression-related tests behind feature flags to avoid invalid builds when compression features are off.
Changes:
- Make
StreamingEncoder/StreamingDecodergeneric over an encodingConfig(with a default config type). - Add constructors for supplying a custom encoding config (and for encoder: optionally a custom
StreamingConfig). - Gate
CompressionStatstests behindcompression-lz4/compression-zstdfeature flags.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/compression_advanced3_test.rs | Gates CompressionStats tests behind compression feature flags. |
| src/streaming/encoder.rs | Makes StreamingEncoder generic over encoding config and adds constructors for custom configs. |
| src/streaming/decoder.rs | Makes StreamingDecoder generic over decoding config and adds a constructor for custom config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn with_config(writer: W, config: StreamingConfig) -> Self { | ||
| Self { | ||
| writer, | ||
| encoding_config: config::standard(), | ||
| config, | ||
| buffer: alloc::vec::Vec::new(), | ||
| items_in_buffer: 0, |
There was a problem hiding this comment.
This does not compile: the config parameter (a StreamingConfig) shadows the imported crate::config module, so config::standard() resolves to a local variable and cannot be used as a path. Rename the parameter (e.g., streaming_config) or call the module via an unshadowed path (e.g., crate::config::standard()).
| pub fn write_item<T: Encode>(&mut self, item: &T) -> Result<()> { | ||
| // Encode item to temporary buffer | ||
| let item_writer = VecWriter::new(); | ||
| let mut encoder = EncoderImpl::new(item_writer, config::standard()); | ||
| let mut encoder = EncoderImpl::new(item_writer, self.encoding_config); | ||
| item.encode(&mut encoder)?; | ||
| let item_bytes = encoder.into_writer().into_vec(); |
There was a problem hiding this comment.
This likely moves self.encoding_config out of &mut self when constructing EncoderImpl, which will either fail to compile (if C isn't Copy) or prevent reuse of the encoder config across multiple calls. Prefer passing a reference if EncoderImpl::new supports it, or require C: Clone and pass self.encoding_config.clone() (or store the config in a shared wrapper like Arc<C>).
| // Create reader from remaining chunk data | ||
| let reader = SliceReader::new(&chunk.data[chunk.offset..]); | ||
| let mut decoder = DecoderImpl::new(reader, config::standard()); | ||
| let mut decoder = DecoderImpl::new(reader, self.config); | ||
| let item = T::decode(&mut decoder)?; |
There was a problem hiding this comment.
Similar to the encoder, this likely moves self.config out of &mut self when creating DecoderImpl. If C isn't Copy, this will not compile and/or will fail on subsequent calls. Consider passing &self.config (if supported) or adding a Clone bound and using self.config.clone() (or storing the config behind Arc<C>).
| /// Create a new streaming decoder with custom configuration. | ||
| /// Allows to create a decoder with specific endianness, integer encoding, and byte limits. | ||
| /// Really usefull for some performance readings (for example when reading the element number n of a p bit object) or to be compatible with some specific encoders. | ||
| pub fn new_with(reader: R, config: C) -> Self { |
There was a problem hiding this comment.
Spelling/grammar in doc comments: 'Allows to create' -> 'Allows creating', 'usefull' -> 'useful'. Consider tightening the sentence for clarity as well.
|
Thanks @Rick-29 — this is a thoughtful contribution, and the new Bug — cursor not restored in let result = self.read_item().and_then(|opt| {
opt.ok_or_else(|| Error::OwnedCustom {
message: format!("Unexpected end of file at index {idx}"),
})
});
self.seek(SeekFrom::Start(current))?;
resultTests: the new public API ( Compression test cfg guards: good catch — the two Minor:
If you can address the |
No description provided.