Added default message and param to OptsFn #116
Conversation
|
Oh, hi, I was asked a question :-D |
|
I understand the use case but I don't think it's common enough (at this point) to warrant the breaking change. Separately, it brings up some questions like if we do a change like this, should To accommodate your scenario without breaking existing users I'd suggest making a middleware layer that you call in your methods instead of calling Ensure directly, and have that layer catch the exceptions from Ensure to rethrow exceptions with your specified type. Catching the An example would be: void Main()
{
Abc("123");
}
// Define other methods and classes here
void Abc(string i)
{
DomainValidation.ArgumentValidationBlock(delegate {
Ensure.That(i).IsNotNull();
});
}
static class DomainValidation
{
public static void ArgumentValidationBlock(Action action)
{
try
{
action();
}
catch(ArgumentException ae)
{
// Ideally you'd also pass ae as the inner exception here anyway
// , to get the stack trace of the original exception from
// Ensure.That. You "never" want to just drop an exception
// TODO: Use your exception type
throw new NullReferenceException(ae.Message, ae /* passing ae as inner exception */);
}
}
} |
|
@ndrwrbgs If taking that middleware route the argument should be passed through, right? Just to get rid of the risk of not being able to cache the closure? E.g. static class DomainValidation
{
public static void ArgumentValidationBlock<T>(T i, Action<T> action)
{
try
{
action(i);
}
catch(ArgumentException ae)
{
// Ideally you'd also pass ae as the inner exception here anyway
// , to get the stack trace of the original exception from
// Ensure.That. You "never" want to just drop an exception
// TODO: Use your exception type
throw new NullReferenceException(ae.Message, ae /* passing ae as inner exception */);
}
}
} |
|
If we populated the |
|
I saw the EnsureOptions as a possible route also, but it looked (currently)
like something that doesn't change on even call to a method (eg with the
value). It's definitely doable though.
Honestly the request is really valid, if I had no other TODOs on the
project I think I'd take this up and look into approaches for custom
exception types/removing the verbosity of OptsFn calls (maybe a generic
argument that had to be inherited from ArgumentException and has to have a
certain ctor signature) but at this moment I'd rather focus energy on more
validation methods and performance (so we can assert "it's just as fast as
'if null throw'").
…On Sun, Feb 23, 2020, 5:35 AM Daniel Wertheim ***@***.***> wrote:
If we populated the EnsureOptions struct with the default message and the
value we would be set right? And that struct would only be created if a
lambda was injected.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#116?email_source=notifications&email_token=ACSHCOSTWBAMHUOJQIGLADDREJ3RTA5CNFSM4HF5T27KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMV36HY#issuecomment-590069535>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACSHCOW2EVGRSUC43RH6YMDREJ3RTANCNFSM4HF5T27A>
.
|
|
What if Then This has no breaking changes for the users. One could think about removal of |
|
As long as it doesn't break the API I'm okay with any approach (since I
don't use the customization methods at least yet). Leaving @danielwertheim
with all the tough decision making responsibility :-P
…On Sun, Feb 23, 2020, 9:29 AM Evaldas ***@***.***> wrote:
What if EnsureOptions looked like this?
public struct EnsureOptions
{
/// <summary>
/// If <see cref="CustomExceptionFactory"/> is defined, this exception will be thrown instead of the
/// standard exceptions for the particular ensure method.
/// Assign using <see cref="WithException"/>.
/// </summary>
public Exception CustomException => CustomExceptionFactory(null);
/// <summary>
/// If defined, this exception will be thrown instead of the
/// standard exceptions for the particular ensure method.
/// Assign using <see cref="WithException"/>.
/// </summary>
public Func<string, Exception> CustomExceptionFactory { get; private set; }
/// <summary>
/// If defined, and no <see cref="CustomExceptionFactory"/> has been defined,
/// this message will be used instead of the standard message for the
/// particular ensure method.
/// Assign using <see cref="WithMessage"/>.
/// </summary>
public string CustomMessage { get; private set; }
public EnsureOptions WithException(Exception ex)
{
CustomExceptionFactory = x => ex;
return this;
}
public EnsureOptions WithException(Func<string, Exception> factory)
{
CustomExceptionFactory = factory;
return this;
}
public EnsureOptions WithMessage(string message)
{
CustomMessage = message;
return this;
}
}
Then ExceptionFactory could be like this:
if (opts.CustomExceptionFactory != null)
return opts.CustomExceptionFactory(defaultMessage);
This has no breaking changes for the users. One could think about removal
of CustomException, or at least add [Obsolete], so nobody would use its
getter.
If this is good enough, I can commit it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#116?email_source=notifications&email_token=ACSHCORLEGYGT3KXCUZUUMDREKW77A5CNFSM4HF5T27KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMWB57Q#issuecomment-590094078>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACSHCOXTOOBBNGVY2EG3QX3REKW77ANCNFSM4HF5T27A>
.
|
|
@danielwertheim any news regarding this PR? |
| /// </summary> | ||
| public Exception CustomException { get; private set; } | ||
| public Func<string, Exception> CustomExceptionFactory { get; private set; } |
There was a problem hiding this comment.
Right now this takes only the original message, right?
I feel like taking the paramname and value would be more important than preserving whatever text the library supplied out of box. Thoughts?
There was a problem hiding this comment.
Yes, it does indeed take only the original message.
Not sure if paramname and value gives any value. During the call to Ensure, I already know both of these. In that case, I could already do this, without this pull request, right?
Ensure.String.HasLengthBetween(name, 0, 10, nameof(Name), o => o.WithException(new DomainValidationException($"The given string \"{name}\" is too long. Must be between '0' and '10'.", nameof(Name))));
However, what I wanted to achieve is not to do anything, just to change the type of the exception, preserving the message.
EDIT:
I had a wild idea - maybe then we should pass the default exception? That way we could keep the message (or even customize it), And one could pass it as inner exception if needed. The factory would look like this:
internal Exception ArgumentException([NotNull] string defaultMessage, string paramName, OptsFn optsFn = null)
{
Exception Generate(string message) => new ArgumentException(message, paramName);
if (optsFn != null)
{
var opts = optsFn(new EnsureOptions());
if (opts.CustomExceptionFactory != null)
return opts.CustomExceptionFactory(Generate(defaultMessage));
if (opts.CustomMessage != null)
return Generate(opts.CustomMessage);
}
return Generate(defaultMessage);
}
| /// standard exceptions for the particular ensure method. | ||
| /// Assign using <see cref="WithException(Exception)"/>. | ||
| /// </summary> | ||
| public Exception CustomException => CustomExceptionFactory(string.Empty); |
There was a problem hiding this comment.
[nit] minor perf hit on method call for folks using this API. As I don't use it, I am not going to be the evangelist that nay-says, but something to keep in mind as the OP is a user of this customization.
There was a problem hiding this comment.
/I advise we not let performance regressions prevent innovation, I feel an obligation to note it for stakeholders, but we can fix issues when(/IF) they are an issue for someone/
There was a problem hiding this comment.
Sorry, but could you explain how this has performance hit? Properties are methods under the hood, AFAIK, so this does not change. The factory also returns straight-away the given exception. What am I missing?
|
@nevaldas thanks for this PR and the contrib. Sorry for the looooooong response time. I kept the Feel free to indicate if I missed any functionality. |
I'd like to offer this change.
The idea of this is to have default message as well as param name in the OptsFn.
In our case, we want to override the thrown exceptions by our ones, so we could easily handle them. However, we don't want to lose the message (which contains all provided values to the ensure).
For instance, if we have:
Ensure.String.HasLengthBetween(name, 0, 10, nameof(Name));The default exception and message would "ArgumentException" and "The string is too long. Must be between '0' and '10'.
But we want to catch only validation errors, so we could show the administrator what the issue is. That means that "ArgumentException" is a bit too risky for us.
So the desired outcome would be exception of our type, let's say, "DomainValidationException" and message to remain the same. With ".WithException" we would need to duplicate the given arguments as well as the message, for example:
Ensure.String.HasLengthBetween(name, 0, 10, nameof(Name), o => o.WithException(new DomainValidationException("The string is too long. Must be between '0' and '10'.", nameof(Name))));Which means that if range changes, it might be that developer wouldn't change the message.
Also, the code looks abit noisy.
The solution with given changes would be:
Ensure.String.HasLengthBetween(name, 0, 10, nameof(Name), EnsureHelper.WithValidationException);Where EnsureHelper would have such property:
public static OptsFn WithValidationException => (options, defaultMessage, paramName) => options.WithException(new DomainValidationException(defaultMessage, paramName));Way cleaner, isn't it?
However, current solution means breaking changes, because it requires to change the OptsFn delegate. I could offer another solution: pass the default message and param name to EnsureOptions' constructor. Then such change would not be a breaking one. I could make such change if needed.