Implement CreateFromDirectory TarEntryFormat overloads #123407
Conversation
dotnet-policy-service
bot
added
the
community-contribution
src/libraries/System.Formats.Tar/tests/TarFile/TarFile.CreateFromDirectory.Format.Tests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarFile/TarFile.CreateFromDirectory.Format.Tests.cs
Outdated
Show resolved
Hide resolved
|
Hello @kasperk81 , looks like we both opened PRs to implement #121819 (mine is #123427) If you’d like to proceed with your changes, I’m happy to close mine and help move the review forward on this PR. |
|
This was opened before yours, so you can close it. I've these changes since nov #121819 (comment) |
src/libraries/System.Formats.Tar/tests/TarFile/TarFile.CreateFromDirectory.Format.Tests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarFile/TarFile.CreateFromDirectory.Format.Tests.cs
Outdated
Show resolved
Hide resolved
kasperk81
force-pushed
the
new-tar-api
branch
from
4f5d483 to
0c3d750
Compare
| string fileName = "file.txt"; | ||
| File.Create(Path.Join(source.Path, fileName)).Dispose(); | ||
|
|
||
| await using MemoryStream archive = new MemoryStream(); |
There was a problem hiding this comment.
very minor nit - we could match the existing style by using block style await using here and through the async tests
| @@ -1,4 +1,4 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
There was a problem hiding this comment.
Can we undo the change here?
| private static async Task CreateFromDirectoryInternalAsync(string sourceDirectoryName, Stream destination, bool includeBaseDirectory, bool leaveOpen, CancellationToken cancellationToken) | ||
| private static async Task CreateFromDirectoryInternalAsync(string sourceDirectoryName, Stream destination, bool includeBaseDirectory, bool leaveOpen, TarEntryFormat format, CancellationToken cancellationToken) | ||
| { | ||
| VerifyCreateFromDirectoryArguments(sourceDirectoryName, destination); |
There was a problem hiding this comment.
What do you think about verifying the format too?
| { | ||
| ArgumentException.ThrowIfNullOrEmpty(sourceDirectoryName); | ||
| ArgumentNullException.ThrowIfNull(destination); | ||
|
|
There was a problem hiding this comment.
Should we also implement a fail-fast behavior for format here? (and aysnc version as well)
There was a problem hiding this comment.
Pull request overview
This PR implements new overloads for TarFile.CreateFromDirectory methods to allow specifying the TarEntryFormat parameter, addressing issue #121819. Currently, these methods only create archives in PAX format, but the underlying TarWriter supports all four tar entry formats (V7, Ustar, Pax, Gnu). These new overloads expose this existing capability to the public API.
Changes:
- Added four new public method overloads to
TarFileclass that accept aTarEntryFormatparameter - Existing methods now delegate to the new overloads with
TarEntryFormat.Paxas the default format - Added test helper methods for valid and invalid tar entry formats
- Added comprehensive test coverage for all four new overloads with both valid and invalid format values
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Formats.Tar/ref/System.Formats.Tar.cs | Added four new public API surface signatures for the new overloads |
| src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarFile.cs | Implemented the new overloads and refactored existing methods to delegate to them with default PAX format |
| src/libraries/System.Formats.Tar/tests/TarTestsBase.cs | Added test data helper methods for valid and invalid TarEntryFormat values |
| src/libraries/System.Formats.Tar/tests/TarFile/TarFile.CreateFromDirectory.Stream.Tests.cs | Added tests for sync stream overload with format parameter |
| src/libraries/System.Formats.Tar/tests/TarFile/TarFile.CreateFromDirectory.File.Tests.cs | Added tests for sync file overload with format parameter |
| src/libraries/System.Formats.Tar/tests/TarFile/TarFile.CreateFromDirectoryAsync.Stream.Tests.cs | Added tests for async stream overload with format parameter |
| src/libraries/System.Formats.Tar/tests/TarFile/TarFile.CreateFromDirectoryAsync.File.Tests.cs | Added tests for async file overload with format parameter |
| public static Task CreateFromDirectoryAsync(string sourceDirectoryName, string destinationFileName, bool includeBaseDirectory, CancellationToken cancellationToken = default) | ||
| => CreateFromDirectoryAsync(sourceDirectoryName, destinationFileName, includeBaseDirectory, TarEntryFormat.Pax, cancellationToken); | ||
|
|
||
| /// <inheritdoc cref="CreateFromDirectoryAsync(string, string, bool, CancellationToken)" /> |
There was a problem hiding this comment.
The new format parameter should have a <param name="format"> documentation tag describing what it does. According to the documentation guidelines in docs.prompt.md, all method parameters should be documented with param tags. The description should be a noun phrase, such as "One of the enumeration values that specifies the tar entry format to use for the archive."
| /// <inheritdoc cref="CreateFromDirectoryAsync(string, string, bool, CancellationToken)" /> | |
| /// <inheritdoc cref="CreateFromDirectoryAsync(string, string, bool, CancellationToken)" /> | |
| /// <param name="format">One of the enumeration values that specifies the tar entry format to use for the archive.</param> |
| public static void CreateFromDirectory(string sourceDirectoryName, Stream destination, bool includeBaseDirectory) | ||
| => CreateFromDirectory(sourceDirectoryName, destination, includeBaseDirectory, TarEntryFormat.Pax); | ||
|
|
||
| /// <inheritdoc cref="CreateFromDirectory(string, Stream, bool)" /> |
There was a problem hiding this comment.
The new format parameter should have a <param name="format"> documentation tag describing what it does. According to the documentation guidelines in docs.prompt.md, all method parameters should be documented with param tags. The description should be a noun phrase, such as "One of the enumeration values that specifies the tar entry format to use for the archive."
| /// <inheritdoc cref="CreateFromDirectory(string, Stream, bool)" /> | |
| /// <inheritdoc cref="CreateFromDirectory(string, Stream, bool)" /> | |
| /// <param name="format">One of the enumeration values that specifies the tar entry format to use for the archive.</param> |
| public static Task CreateFromDirectoryAsync(string sourceDirectoryName, Stream destination, bool includeBaseDirectory, CancellationToken cancellationToken = default) | ||
| => CreateFromDirectoryAsync(sourceDirectoryName, destination, includeBaseDirectory, TarEntryFormat.Pax, cancellationToken); | ||
|
|
||
| /// <inheritdoc cref="CreateFromDirectoryAsync(string, Stream, bool, CancellationToken)" /> |
There was a problem hiding this comment.
The new format parameter should have a <param name="format"> documentation tag describing what it does. According to the documentation guidelines in docs.prompt.md, all method parameters should be documented with param tags. The description should be a noun phrase, such as "One of the enumeration values that specifies the tar entry format to use for the archive."
| /// <inheritdoc cref="CreateFromDirectoryAsync(string, Stream, bool, CancellationToken)" /> | |
| /// <inheritdoc cref="CreateFromDirectoryAsync(string, Stream, bool, CancellationToken)" /> | |
| /// <param name="format">One of the enumeration values that specifies the tar entry format to use for the archive.</param> |
| /// <inheritdoc cref="CreateFromDirectory(string, string, bool)" /> | ||
| /// <exception cref="ArgumentOutOfRangeException"><paramref name="format"/> is either <see cref="TarEntryFormat.Unknown"/>, or not one of the other enum values.</exception> |
There was a problem hiding this comment.
The new format parameter should have a <param name="format"> documentation tag describing what it does. According to the documentation guidelines in docs.prompt.md, all method parameters should be documented with param tags. The description should be a noun phrase, such as "One of the enumeration values that specifies the tar entry format to use for the archive."
iremyux
left a comment
iremyux
left a comment
There was a problem hiding this comment.
looks good overall - some minor comments about checks for format value
4 participants
fix #121819