Make SharpCompressStream public#1233
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 7 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
SUGGESTION
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (6 files)
|
There was a problem hiding this comment.
Pull request overview
This PR exposes SharpCompressStream as a public API type (including its partial class files), improves the XML documentation around stream creation/wrapping behavior, and makes small related visibility/tooling adjustments.
Changes:
- Made
SharpCompressStreamand its partial definitionspublic. - Expanded XML docs for
SharpCompressStream.Create(...)/CreateNonDisposing(...)to better describe passthrough vs rewindable behavior and ownership semantics. - Restricted
ThrowOnDisposevisibility and adjustedSeekableSharpCompressStreamaccordingly; updated repo VS Code settings.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/SharpCompress/IO/SharpCompressStream.cs |
Makes the core stream wrapper public and narrows ThrowOnDispose visibility. |
src/SharpCompress/IO/SharpCompressStream.Create.cs |
Makes factory helpers public and significantly expands XML documentation for stream creation behavior. |
src/SharpCompress/IO/SharpCompressStream.Async.cs |
Makes async partial definition public to match the now-public type. |
src/SharpCompress/IO/SeekableSharpCompressStream.cs |
Removes the ThrowOnDispose override now that the base member is no longer public/virtual. |
.vscode/settings.json |
Adds a VS Code terminal tool auto-approval setting for dotnet csharpier. |
Comments suppressed due to low confidence (1)
src/SharpCompress/IO/SharpCompressStream.cs:46
- Now that SharpCompressStream is public, the public constructor should validate the input stream. Currently
this.stream = stream;allowsnull, which will lead to a NullReferenceException later; prefer throwingArgumentNullExceptionimmediately to make the public API safer and consistent with other stream wrappers in this repo.
internal bool ThrowOnDispose { get; set; }
public SharpCompressStream(Stream stream)
{
this.stream = stream;
_logicalPosition = 0;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <para><b>Non-seekable streams</b> (network streams, compressed streams, pipes) — allocates | ||
| /// a ring buffer of <paramref name="bufferSize"/> bytes. All bytes read from the underlying | ||
| /// stream are kept in the ring buffer so that <see cref="Rewind()"/> can replay them without | ||
| /// re-reading the underlying stream. If more bytes have been read than the ring buffer can hold, | ||
| /// a subsequent rewind will throw <see cref="InvalidOperationException"/>; increase | ||
| /// <paramref name="bufferSize"/> or <see cref="Common.Constants.RewindableBufferSize"/> to | ||
| /// avoid this.</para> |
There was a problem hiding this comment.
The XML docs claim that for non-seekable streams a ring buffer of bufferSize bytes is allocated (defaulting to Constants.RewindableBufferSize). However, the current implementation can end up allocating the buffer later via StartRecording() using Constants.BufferSize, and if bufferSize is null the non-seekable path may not allocate a ring buffer up-front. Please align docs and implementation so the actual buffer size/behavior matches what is documented (ideally honoring Constants.RewindableBufferSize when bufferSize is null).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@adamhathcock I've opened a new pull request, #1234, to work on those changes. Once the pull request is ready, I'll request review from you. |
…reate() and StartRecording() Co-authored-by: adamhathcock <527620+adamhathcock@users.noreply.github.com>
Fix SharpCompressStream.Create() buffer size misalignment for non-seekable streams
This pull request primarily makes the
SharpCompressStreamclass and its related partial classes public, updates documentation for stream creation methods, and adjusts visibility of certain members. These changes improve API accessibility and clarity, and refine internal stream management.API accessibility improvements:
SharpCompressStreamand its partial classes frominternaltopublic, allowing them to be used outside the assembly. (src/SharpCompress/IO/SharpCompressStream.cs,src/SharpCompress/IO/SharpCompressStream.Create.cs,src/SharpCompress/IO/SharpCompressStream.Async.cs) [1] [2] [3]Documentation and usability enhancements:
CreateandCreateNonDisposingfactory methods inSharpCompressStream, detailing passthrough and recording behaviors, ownership semantics, and buffer usage. (src/SharpCompress/IO/SharpCompressStream.Create.cs)Internal API refinements:
ThrowOnDisposeproperty inSharpCompressStreamfrom public to internal, restricting its usage to within the assembly. (src/SharpCompress/IO/SharpCompressStream.cs)ThrowOnDisposeinSeekableSharpCompressStream, simplifying stream disposal handling. (src/SharpCompress/IO/SeekableSharpCompressStream.cs)Development tooling:
dotnet csharpiertool in terminal tasks. (.vscode/settings.json)