Skip to content

Add missing System.Formats.Tar docs #74786

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
carlossanlop wants to merge 2 commits into from
Closed

Add missing System.Formats.Tar docs #74786

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47dbcd1
Add missing System.Formats.Tar docs
carlossanlop Aug 29, 2022
7185888
Apply suggestions from code review
carlossanlop Aug 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/libraries/System.Formats.Tar/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,8 @@
<data name="TarHardLinkToDirectoryNotAllowed" xml:space="preserve">
<value>Cannot create the hard link '{0}' targeting the directory '{1}'.</value>
</data>
<data name="TarInvalidFormat" xml:space="preserve">
<value>The archive format is invalid: '{0}'</value>
<data name="TarInvalidEntryFormat" xml:space="preserve">
<value>The entry format is invalid: '{0}'</value>
</data>
<data name="TarPosixFormatExpected" xml:space="preserve">
<value>A POSIX format was expected (Ustar or PAX), but could not be reliably determined for entry '{0}'.</value>
Expand Down
5 changes: 5 additions & 0 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/GnuTarEntry.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ public GnuTarEntry(TarEntryType entryType, string entryName)
/// <summary>
/// Initializes a new <see cref="GnuTarEntry"/> instance by converting the specified <paramref name="other"/> entry into the GNU format.
/// </summary>
/// <param name="other">The <see cref="TarEntry"/> instance to convert to the GNU format.</param>
/// <exception cref="InvalidOperationException"><para><paramref name="other"/> is a <see cref="PaxGlobalExtendedAttributesTarEntry"/> instance.</para>
Copy link
Member

@jozkee jozkee Aug 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be ArgumentException? InvalidOperationException conveys a state error.

The exception that is thrown when a method call is invalid for the object's current state.

Copy link
Contributor Author

@carlossanlop carlossanlop Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's what TarHelpers throws in the base.

Copy link
Contributor Author

@carlossanlop carlossanlop Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not possible to convert a PAX GEA entry to any other type. So your code is in an invalid state at this point for attempting that.

Copy link
Member

@jozkee jozkee Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the object's current state, not the code's. There's no object until the ctor returns, I think it is even wrong to throw InvalidOperation in a ctor.

Copy link
Member

@jozkee jozkee Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @bartonjs as we recently discussed similar cases.

Copy link
Member

@jozkee jozkee Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering: if we change the exception type, it wouldn't meet the 7.0 servicing bar and if we change it on 8.0, it would imply a breaking change (which may not be worth it?). So this is not actionable until proven otherwise (customer feedback, etc.).
cc @danmoseley, is my reasoning correct?

Copy link
Member

@bartonjs bartonjs Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jozkee is correct, in new code this should be an ArgumentException because

  • Your exception text blames the cause of the exception on a parameter/argument, and
  • It's easy enough for the caller to have known it was going to throw.

InvalidOperationException means "you asked an object to do something, and that makes no sense right now". That could be "I'm in readonly mode and you invoked a mutation member", or "this method can't be called until some property is set" or "this method can't be called because you passed null in the ctor", or whatever.

HAL's "I'm afraid I can't do that, Dave." is InvalidOperationException thrown from hal.OpenDoors(DoorTypes.PodBay)... "I" can't do that, because of something that happened outside of this request. hal.OpenDoors(DoorTypes.PodBay | DoorTypes.EmergencyAirlockExterior) might throw an ArgumentException that only a single value can be requested, and that would, in HAL-ese, be "You know I can only open one kind of door at a time, Dave." (the error is Dave's, so ArgumentException)).

If @danmoseley 's Dan-mocracy is a De-mocracy, then I'll vote for fixing the exception type in the product code.

Copy link
Contributor Author

@carlossanlop carlossanlop Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InvalidOperationException means "you asked an object to do something, and that makes no sense right now".

The user is asking to convert this PaxGEA entry to another type, and that doesn't make sense right now, or ever.

Your exception text blames the cause of the exception on a parameter/argument, and

So maybe the text should be changed?

Copy link
Member

@bartonjs bartonjs Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't make sense ... ever.

Right, so InvalidOperationException is the wrong exception. The exception isn't conditional on the target object's state (no fields/properties were consulted before the throw)

https://docs.microsoft.com/en-us/dotnet/api/system.invalidoperationexception?view=net-6.0

The exception that is thrown when a method call is invalid for the object's current state.

and from the remarks:

InvalidOperationException is used in cases when the failure to invoke a method is caused by reasons other than invalid arguments.

Since the reason here is entirely invalid arguments (the combination of "this method" (constructor) and "this argument" will NEVER work), it should be an ArgumentException.

Copy link
Contributor Author

@carlossanlop carlossanlop Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I submitted it as a separate change to keep it as minimal as possible for a potential backport: #74845

The documentation changes in this PR should not matter for RC2, since intellisense will come from whatever we put in dotnet-api-docs.

/// <para>-or-</para>
/// <para>The entry type of <paramref name="other"/> is not supported in the GNU format.</para></exception>
Copy link
Member

@jozkee jozkee Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same for this exception.

/// <exception cref="FormatException">The format of <paramref name="other"/> is invalid.</exception>
public GnuTarEntry(TarEntry other)
: base(other, TarEntryFormat.Gnu)
{
Expand Down
5 changes: 5 additions & 0 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/PaxTarEntry.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,11 @@ public PaxTarEntry(TarEntryType entryType, string entryName, IEnumerable<KeyValu
/// <summary>
/// Initializes a new <see cref="PaxTarEntry"/> instance by converting the specified <paramref name="other"/> entry into the PAX format.
/// </summary>
/// <param name="other">The <see cref="TarEntry"/> instance to convert to the PAX format.</param>
/// <exception cref="InvalidOperationException"><para><paramref name="other"/> is a <see cref="PaxGlobalExtendedAttributesTarEntry"/> instance.</para>
/// <para>-or-</para>
/// <para>The entry type of <paramref name="other"/> is not supported in the PAX format.</para></exception>
/// <exception cref="FormatException">The format of <paramref name="other"/> is invalid.</exception>
public PaxTarEntry(TarEntry other)
: base(other, TarEntryFormat.Pax)
{
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ TarEntryType.RegularFile or

case TarEntryFormat.Unknown:
default:
throw new FormatException(string.Format(SR.TarInvalidFormat, archiveFormat));
throw new FormatException(string.Format(SR.TarInvalidEntryFormat, archiveFormat));
}

throw new InvalidOperationException(string.Format(SR.TarEntryTypeNotSupportedInFormat, entryType, archiveFormat));
Expand Down
5 changes: 2 additions & 3 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,8 @@ public void Dispose()
}
}

/// <summary>
/// Asynchronously disposes the current <see cref="TarReader"/> instance, and disposes the streams of all the entries that were read from the archive.
/// </summary>
/// <summary>Asynchronously disposes the current <see cref="TarReader"/> instance, and closes the streams of all the entries that were read from the archive if the <c>leaveOpen</c> argument was set to <see langword="false"/> in the constructor.</summary>
/// <returns>A value task that represents the asynchronous dispose operation.</returns>
/// <remarks>The <see cref="TarEntry.DataStream"/> property of any entry can be replaced with a new stream. If the user decides to replace it on a <see cref="TarEntry"/> instance that was obtained using a <see cref="TarReader"/>, the underlying stream gets disposed immediately, freeing the <see cref="TarReader"/> of origin from the responsibility of having to dispose it.</remarks>
public async ValueTask DisposeAsync()
{
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarWriter.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ private TarEntry ConstructEntryForWriting(string fullPath, string entryName, Fil
TarEntryFormat.Ustar => new UstarTarEntry(entryType, entryName),
TarEntryFormat.Pax => new PaxTarEntry(entryType, entryName),
TarEntryFormat.Gnu => new GnuTarEntry(entryType, entryName),
_ => throw new FormatException(string.Format(SR.TarInvalidFormat, Format)),
_ => throw new FormatException(string.Format(SR.TarInvalidEntryFormat, Format)),
};

if (entryType is TarEntryType.BlockDevice or TarEntryType.CharacterDevice)
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarWriter.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ private TarEntry ConstructEntryForWriting(string fullPath, string entryName, Fil
TarEntryFormat.Ustar => new UstarTarEntry(entryType, entryName),
TarEntryFormat.Pax => new PaxTarEntry(entryType, entryName),
TarEntryFormat.Gnu => new GnuTarEntry(entryType, entryName),
_ => throw new FormatException(string.Format(SR.TarInvalidFormat, Format)),
_ => throw new FormatException(string.Format(SR.TarInvalidEntryFormat, Format)),
};

FileSystemInfo info = (attributes & FileAttributes.Directory) != 0 ? new DirectoryInfo(fullPath) : new FileInfo(fullPath);
Expand Down
10 changes: 5 additions & 5 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,8 @@ public void Dispose()
}
}

/// <summary>
/// Asynchronously disposes the current <see cref="TarWriter"/> instance, and closes the archive stream if the <c>leaveOpen</c> argument was set to <see langword="false"/> in the constructor.
/// </summary>
/// <summary>Asynchronously disposes the current <see cref="TarWriter"/> instance, and closes the archive stream if the <c>leaveOpen</c> argument was set to <see langword="false"/> in the constructor.</summary>
/// <returns>A value task that represents the asynchronous dispose operation.</returns>
public async ValueTask DisposeAsync()
{
if (!_isDisposed)
Expand Down Expand Up @@ -224,6 +223,7 @@ public void WriteEntry(TarEntry entry)
/// </summary>
/// <param name="entry">The tar entry to write.</param>
/// <param name="cancellationToken">The token to monitor for cancellation requests. The default value is <see cref="CancellationToken.None" />.</param>
/// <returns>A task that represents the asynchronous write operation.</returns>
/// <remarks><para>Before writing an entry to the archive, if you wrote data into the entry's <see cref="TarEntry.DataStream"/>, make sure to rewind it to the desired start position.</para>
/// <para>These are the entry types supported for writing on each format:</para>
/// <list type="bullet">
Expand Down Expand Up @@ -298,7 +298,7 @@ private void WriteEntryInternal(TarEntry entry)

default:
Debug.Assert(entry.Format == TarEntryFormat.Unknown, "Missing format handler");
throw new FormatException(string.Format(SR.TarInvalidFormat, Format));
throw new FormatException(string.Format(SR.TarInvalidEntryFormat, Format));
}

_wroteEntries = true;
Expand All @@ -320,7 +320,7 @@ private async Task WriteEntryAsyncInternal(TarEntry entry, CancellationToken can
TarEntryFormat.Pax when entry._header._typeFlag is TarEntryType.GlobalExtendedAttributes => entry._header.WriteAsPaxGlobalExtendedAttributesAsync(_archiveStream, buffer, _nextGlobalExtendedAttributesEntryNumber++, cancellationToken),
TarEntryFormat.Pax => entry._header.WriteAsPaxAsync(_archiveStream, buffer, cancellationToken),
TarEntryFormat.Gnu => entry._header.WriteAsGnuAsync(_archiveStream, buffer, cancellationToken),
_ => throw new FormatException(string.Format(SR.TarInvalidFormat, Format)),
_ => throw new FormatException(string.Format(SR.TarInvalidEntryFormat, Format)),
};
await task.ConfigureAwait(false);

Expand Down
5 changes: 5 additions & 0 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/UstarTarEntry.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ public UstarTarEntry(TarEntryType entryType, string entryName)
/// <summary>
/// Initializes a new <see cref="UstarTarEntry"/> instance by converting the specified <paramref name="other"/> entry into the Ustar format.
/// </summary>
/// <param name="other">The <see cref="TarEntry"/> instance to convert to the Ustar format.</param>
/// <exception cref="InvalidOperationException"><para><paramref name="other"/> is a <see cref="PaxGlobalExtendedAttributesTarEntry"/> instance.</para>
/// <para>-or-</para>
/// <para>The entry type of <paramref name="other"/> is not supported in the Ustar format.</para></exception>
/// <exception cref="FormatException">The format of <paramref name="other"/> is invalid.</exception>
public UstarTarEntry(TarEntry other)
: base(other, TarEntryFormat.Ustar)
{
Expand Down
5 changes: 5 additions & 0 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/V7TarEntry.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ public V7TarEntry(TarEntryType entryType, string entryName)
/// <summary>
/// Initializes a new <see cref="V7TarEntry"/> instance by converting the specified <paramref name="other"/> entry into the V7 format.
/// </summary>
/// <param name="other">The <see cref="TarEntry"/> instance to convert to the V7 format.</param>
/// <exception cref="InvalidOperationException"><para><paramref name="other"/> is a <see cref="PaxGlobalExtendedAttributesTarEntry"/> instance.</para>
/// <para>-or-</para>
/// <para>The entry type of <paramref name="other"/> is not supported in the V7 format.</para></exception>
/// <exception cref="FormatException">The format of <paramref name="other"/> is invalid.</exception>
public V7TarEntry(TarEntry other)
: base(other, TarEntryFormat.V7)
{
Expand Down