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

Correct Expression signature #366

Open
Ergamon opened this issue Dec 10, 2024 · 2 comments
Open

Correct Expression signature #366

Ergamon opened this issue Dec 10, 2024 · 2 comments

Comments

@Ergamon
Copy link
Contributor

Ergamon commented Dec 10, 2024

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?

@ardalis
Copy link
Owner

ardalis commented Dec 10, 2024

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

@Ergamon
Copy link
Contributor Author

Ergamon commented Dec 11, 2024

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?

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

No branches or pull requests

2 participants