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

Prevent string resource lookup if not required #265

Conversation

campbellhardingdeason
Copy link
Contributor

Fixes #264

Signed-off-by: Campbell Harding-Deason <campbell.harding-deason@firstaml.com>
Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Thanks very much for spotting this, and for the PR. Just one small requested change, if you're happy to make it.

/// <param name="condition">The condition to validate; this method will throw an <see cref="ArgumentException"/> if this is false.</param>
/// <param name="paramName">The name of the parameter being validated. May be null.</param>
/// <param name="messageFunc">A func that returns the message to use in the exception, if one is thrown.</param>
public static void CheckArgument([DoesNotReturnIf(false)] bool condition, string paramName, Func<string> messageFunc)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we keep this internal to start with? It looks like we only need it to be internal for now - we're not using resource strings elsewhere, and I suspect other users won't be doing so either, so I'd prefer to avoid exposing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing 👍

Signed-off-by: Campbell Harding-Deason <campbell.harding-deason@firstaml.com>
Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Thanks - I'll squash and merge down into one commit.
I'll try to get a release done next week.

@jskeet jskeet merged commit afb9b85 into cloudevents:main Sep 1, 2023
2 checks passed
@campbellhardingdeason campbellhardingdeason deleted the prevent-unneeded-resource-lookup branch September 1, 2023 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

String resource is looked up eagerly for validation methods
2 participants