Expose SharpCompressStream to allow ringbuffer to be used on non-seekable streams#1220
Expose SharpCompressStream to allow ringbuffer to be used on non-seekable streams#1220adamhathcock merged 3 commits intomasterfrom
Conversation
Code Review SummaryStatus: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (5 files)
|
There was a problem hiding this comment.
Pull request overview
This pull request addresses issue #936 by making SharpCompressStream public, which allows users to wrap non-seekable streams (like HTTP response streams) with a ring buffer before passing them to decompressors like XZStream. The PR includes comprehensive test coverage for both synchronous and asynchronous scenarios with empty and non-empty compressed data.
Changes:
- Made
SharpCompressStreamclass public to enable wrapping of non-seekable streams - Added test coverage for XZStream with non-seekable streams in both sync and async scenarios
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/SharpCompress/IO/SharpCompressStream.cs | Changed class visibility from internal to public |
| src/SharpCompress/IO/SharpCompressStream.Create.cs | Changed partial class visibility from internal to public |
| src/SharpCompress/IO/SharpCompressStream.Async.cs | Changed partial class visibility from internal to public |
| tests/SharpCompress.Test/Xz/XZStreamTests.cs | Added tests for non-seekable stream scenarios (regular and empty) |
| tests/SharpCompress.Test/Xz/XZStreamAsyncTests.cs | Added async tests for non-seekable stream scenarios (regular and empty) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| ```csharp | ||
| // Wrap a non-seekable stream with buffering | ||
| using (var bufferedStream = new SharpCompressStream(rawStream)) |
There was a problem hiding this comment.
WARNING: Buffered example uses the direct constructor, which doesn't enable ring buffering
SharpCompressStream(Stream) doesn't allocate a ring buffer, so the example won't provide the buffering described in the text. Use SharpCompressStream.Create(...) (or pass a buffer size) to ensure the wrapper is configured for non-seekable buffering.
| using (var bufferedStream = new SharpCompressStream(rawStream)) | |
| using (var bufferedStream = SharpCompressStream.Create(rawStream)) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// (such as passthrough vs buffered mode and buffer size) for the underlying | ||
| /// stream and usage scenario. | ||
| /// </remarks> | ||
| public partial class SharpCompressStream : Stream, IStreamStack |
There was a problem hiding this comment.
The public constructor at line 55 (public SharpCompressStream(Stream stream)) lacks XML documentation. According to the class-level remarks, users should prefer using SharpCompressStream.Create(...) methods instead of directly constructing instances. Consider adding XML documentation that explains this constructor's purpose and recommends using the Create methods for most use cases.
|
|
||
| ```csharp | ||
| // Wrap a non-seekable stream with buffering | ||
| using (var bufferedStream = new SharpCompressStream(rawStream)) |
There was a problem hiding this comment.
The documentation example uses the SharpCompressStream constructor directly (new SharpCompressStream(rawStream)), but according to the class-level documentation in SharpCompressStream.cs, users should prefer using the static SharpCompressStream.Create(...) methods. Consider updating this example to use SharpCompressStream.Create(rawStream) instead to align with the recommended usage pattern and ensure proper configuration (seekable vs buffered mode) is selected automatically.
| using (var bufferedStream = new SharpCompressStream(rawStream)) | |
| using (var bufferedStream = SharpCompressStream.Create(rawStream)) |
fixes #936
This pull request makes the
SharpCompressStreamclass public and adds new tests to ensure thatXZStreamcan handle non-seekable streams, including both non-empty and empty cases. These changes improve the public API's usability and test coverage for edge cases involving stream types.Public API changes
SharpCompressStreamand its partial class declarations frominternaltopublic, making it accessible outside its assembly. [1] [2] [3]Test improvements for non-seekable streams
XZStreamTestsandXZStreamAsyncTeststo verify thatXZStreamcan correctly read from non-seekable streams, both for regular and empty compressed data. [1] [2]