Skip to content

Conversation

@carlossanlop
Copy link
Contributor

No description provided.

@carlossanlop carlossanlop self-assigned this Aug 29, 2022
@ghost ghost added the area-System.IO label Aug 29, 2022
@ghost
Copy link

ghost commented Aug 29, 2022

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: carlossanlop
Assignees: carlossanlop
Labels:

area-System.IO

Milestone: -

/// 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

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

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

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

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

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

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

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

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

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

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.

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

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.

@carlossanlop
Copy link
Contributor Author

On a second thought, I'll include the docs changes in the other PR, since I'm adjusting all the exceptions. I'll close this in favor of #74845.

@carlossanlop carlossanlop deleted the TarDocs branch August 31, 2022 01:57
@ghost ghost locked as resolved and limited conversation to collaborators Sep 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants