Skip to content
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.

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

ArgumentOutOfRangeException: use corresponding default message for all overloads #89846

Closed
wants to merge 3 commits into from

Conversation

OwnageIsMagic
Copy link
Contributor

Closes #89783

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 2, 2023
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 2, 2023
}

: this(paramName, null, message) {}

public ArgumentOutOfRangeException(string? message, Exception? innerException)
Copy link
Member

Choose a reason for hiding this comment

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

This is missing the same treatment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@stephentoub stephentoub Aug 2, 2023

Choose a reason for hiding this comment

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

But it's inconsistent with itself. There's no reason a null message to the different ctors should use a different message. Either we change them all on this type or we don't change any on this type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about other classes (string, Exception) ctor? Another PR?

Copy link
Member

Choose a reason for hiding this comment

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

If there are other types where this would make sense, we can use this PR to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So there is no point constraining this to CoreLib.
This will affect all exception classes CoreLib (ls -1 **/*Exception.cs | wc -l # -> 100) and some in other libs
i.e.

public DataException(string? s) : base(s)
{
HResult = HResults.Data;
}
public DataException(string? s, Exception? innerException) : base(s, innerException) { }

public IdentityNotMappedException(string? message)
: base(message)
{
}
public IdentityNotMappedException(string? message, Exception? inner)
: base(message, inner)
{
}
internal IdentityNotMappedException(string? message, IdentityReferenceCollection? unmappedIdentities)
: this(message)

117 classes, but not every Exception out here have custom message


Are you ok with proposed pattern with constructor delegation?

Copy link
Member

Choose a reason for hiding this comment

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

Are you ok with proposed pattern with constructor delegation?

I'd prefer we just add ?? SR.Whatever where each message is passed along to the base.

Delegating from ctor to ctor unnecessarily keeps all larger constructors rooted and prevents them from being trimmed away when the smaller one is being used. It's also a lot more churn.

@OwnageIsMagic
Copy link
Contributor Author

@stephentoub test timeout on arm seems unrelated

@OwnageIsMagic
Copy link
Contributor Author

@stephentoub ping ?

@OwnageIsMagic OwnageIsMagic deleted the patch-1 branch August 14, 2023 02:47
@OwnageIsMagic
Copy link
Contributor Author

Reverted original patch and rebased on fresh main #90505

@ghost ghost locked as resolved and limited conversation to collaborators Sep 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: ArgumentOutOfRangeException constructor with actualValue and system message
2 participants