GuardClauses
GuardClauses copied to clipboard
Correct Expression signature
The Expression of T method seems illogical to me.
public static T Expression<T>(this IGuardClause guardClause,
Func<T, bool> func,
T input,
string message,
[CallerArgumentExpression("input")] string? parameterName = null,
Func<Exception>? exceptionCreator = null)
where T : struct
There are 2 problems in my opinion.
- The message is mandatory, but not used if you use the exceptionCreator method
- If you want to create your own exception, then you are forced to specify the parameterName
So I would propose to split this method in 2 overloads:
public static T Expression<T>(this IGuardClause guardClause,
Func<T, bool> func,
T input,
string message,
[CallerArgumentExpression("input")] string? parameterName = null)
where T : struct
and
public static T Expression<T>(this IGuardClause guardClause,
Func<T, bool> func,
T input,
Func<Exception> exceptionCreator,
[CallerArgumentExpression("input")] string? parameterName = null)
where T : struct
(Same goes for EpressionAsync)
What do you think?
Agreed. Some things were missed when the exception creator overloads were added. Want to PR it?
I did create a pull request for this reworking the signatures as I think they should be.
In the tests I currently only made changes to make them compile (and succeed) again.
When I am already in these files, I have some questions about things I would also like to change, but it is your code ;)
- Why do we have the struct type constraint? The could could also be used for reference types
- The Unit test project uses vanilla xUnit assertions. Maybe a matter of taste, but I would prefer using FluentAssertions. I understand that the dependencies should be kept low for the build result, but for tests it should not matter, does it?
- The tests do not cover the message creation, but test different types of structs, which is reduntant code in my opinion
- There are no tests for the async methods
- To not turn everything upside down I currently only changed the ExpressionAsync signature with the 2 overloads. Shouldnt the exception factory in the async method also be async?