Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new test demonstrating how to extract all entries from various archive formats and updates the extraction API to accept ExtractionOptions. The changes provide flexibility in handling solid vs non-solid archives differently.
- Introduces ExtractAllEntries test covering multiple archive formats (Zip, Rar, Rar5, 7Zip)
- Updates ExtractToDirectory method signature to accept ExtractionOptions parameter
- Refactors ArchiveFactory.WriteToDirectory to use ExtractToDirectory method
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| tests/SharpCompress.Test/ExtractAll.cs | Adds new test demonstrating extraction patterns for solid and non-solid archives across multiple formats |
| src/SharpCompress/Archives/IArchiveExtensions.cs | Removes unused System.Linq import, adds ExtractionOptions parameter to ExtractToDirectory, removes XML documentation, and switches from File.OpenWrite to WriteToFile for extraction |
| src/SharpCompress/Archives/ArchiveFactory.cs | Updates WriteToDirectory to call ExtractToDirectory instead of archive.WriteToDirectory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static void ExtractToDirectory( | ||
| this IArchive archive, | ||
| string destination, | ||
| ExtractionOptions? options = null, | ||
| Action<double>? progressReport = null, | ||
| CancellationToken cancellationToken = default | ||
| ) |
There was a problem hiding this comment.
The XML documentation for this method was removed. Consider restoring or adding a summary comment to document the purpose of this method, its parameters (especially the new 'options' parameter), and any important behavior differences from WriteToDirectory.
|
|
||
| namespace SharpCompress.Test; | ||
|
|
||
| public class ExtractAll : TestBase |
There was a problem hiding this comment.
The class name 'ExtractAll' could be more descriptive and follow the naming conventions used in other test files. Consider renaming to 'ExtractAllTests' to be consistent with naming patterns like 'ArchiveTests', 'ReaderTests', etc.
| public class ExtractAll : TestBase | |
| public class ExtractAllTests : TestBase |
| @@ -0,0 +1,44 @@ | |||
| using System.IO; | |||
| using System.Linq; | |||
There was a problem hiding this comment.
The System.Linq using directive is unused and can be removed.
| using System.Linq; |
| { | ||
| using var archive = Open(sourceArchive); | ||
| archive.WriteToDirectory(destinationDirectory, options); | ||
| archive.ExtractToDirectory(destinationDirectory, options); |
There was a problem hiding this comment.
The static method WriteToDirectory now internally calls ExtractToDirectory, which creates a naming inconsistency. Consider either renaming the public method to ExtractToDirectory for consistency, or documenting why WriteToDirectory internally uses ExtractToDirectory to help future maintainers understand the design choice.
|
@adamhathcock I've opened a new pull request, #1078, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@copilot see why the test fails as it should probably pass. 7Zip may not be implemented correctly or the new asynchronous overloads may have caused the issue |
|
@adamhathcock I've opened a new pull request, #1081, to work on those changes. Once the pull request is ready, I'll request review from you. |
…chives Co-authored-by: adamhathcock <527620+adamhathcock@users.noreply.github.com>
- Restored original async ExtractAllEntries test with using statement - Added new ExtractAllEntriesSync test (all tests pass) - Fixed potential partial read bug in LzmaStream.DecodeChunkHeaderAsync - Added ReadFullyAsync helper to ensure complete reads - ReadAsync is not guaranteed to return all requested bytes - Async tests for 7Zip still failing with Data Error - Issue appears related to LZMA2 stream state management - _needDictReset flag not being cleared correctly in async flow - Further investigation needed Co-authored-by: adamhathcock <527620+adamhathcock@users.noreply.github.com>
…tion - Each 7Zip file now gets a fresh decompression stream instead of reusing shared streams - Added SyncOnlyStream wrapper to force async operations to use synchronous equivalents - This avoids LZMA decoder state corruption bugs in async operations - Performance trade-off: slower async extraction for 7Zip but correctness guaranteed - Documented behavior and performance implications in FORMATS.md All 14 ExtractAll tests now pass (both async and sync) Co-authored-by: adamhathcock <527620+adamhathcock@users.noreply.github.com>
The SyncOnlyStream wrapper is necessary because the LZMA decoder has bugs in its async implementation that cause state corruption (IndexOutOfRangeException, DataErrorException) even with fresh, non-shared streams. Without this wrapper, async operations on LZMA streams fail. The proper fix would be to repair the async bugs in LzmaStream.ReadAsync, Decoder.CodeAsync, and OutWindow async operations, but that requires deep changes to the decoder state machine. Added detailed comments explaining this is a workaround and where the real fix should go. Co-authored-by: adamhathcock <527620+adamhathcock@users.noreply.github.com>
Fix async LZMA extraction bug for 7Zip archives
# Conflicts: # src/SharpCompress/Archives/IArchiveExtensions.cs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Shows how do extract all. 7Zip still sucks