GuardClauses icon indicating copy to clipboard operation
GuardClauses copied to clipboard

Correct Expression signature

Open Ergamon opened this issue 1 year ago • 2 comments

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.

  1. The message is mandatory, but not used if you use the exceptionCreator method
  2. 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?

Ergamon avatar Dec 10 '24 19:12 Ergamon

Agreed. Some things were missed when the exception creator overloads were added. Want to PR it?

ardalis avatar Dec 10 '24 21:12 ardalis

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 ;)

  1. Why do we have the struct type constraint? The could could also be used for reference types
  2. 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?
  3. The tests do not cover the message creation, but test different types of structs, which is reduntant code in my opinion
  4. There are no tests for the async methods
  5. 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?

Ergamon avatar Dec 11 '24 08:12 Ergamon