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

Refactor AgainstExpression to Expression for Clarity and Consistency #317

Merged

Conversation

SimonNyvall
Copy link
Contributor

@SimonNyvall SimonNyvall commented Nov 23, 2023

Fix: Update AgainstExpression to Expression

Summary: This PR addresses issue #309. The changes include renaming AgainstExpression to Expression, marking the former as obsolete, and ensuring that the new method clearly indicates that the expression should describe the INVALID state.

Changes Made:

  • Deprecated AgainstExpression method and marked it as [Obsolete].
  • Renamed the GuardAgainstExpressionExtenssion to GuardAgainstExpressionExtenssionDeprecated
  • Created AgainstExpressionExtenssion with CallerArgumentExpression
  • Incorporated CallerArgumentExpression attribute for better clarity in exception messages.
  • Updated documentation and comments to clearly indicate that the expression parameter should describe the invalid case, and when it evaluates to true, it implies the input is in an invalid state.

Testing:

  • Added unit tests for the new Expression method covering various scenarios.
  • Renamed the GuardAgainstExpression to GuardAgainstExpressionDeprecated
  • Manually tested to ensure backward compatibility.

Checklist:

  • Deprecated AgainstExpression method.
  • Implemented new Expression method.
  • Updated unit tests.
  • Updated documentation.

@ardalis Please review the changes and provide your insights.

Copy link
Owner

@ardalis ardalis left a comment

Choose a reason for hiding this comment

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

Your comments say the expression will throw if the func evaluates to true but your actual code and tests say that it should throw when the func is false. Keep the comments and change the code to match.

Revised Guard.Against.Expression and Guard.Against.ExpressionAsync to throw ArgumentException when expressions evaluate to true, aligning with the documentation. This change ensures consistency and correctness in method behavior.
Refactored unit tests for Guard.Against.Expression to match updated method logic. Tests now correctly handle scenarios where expressions evaluate to true, with adjustments to test conditions and error messages for clarity.
@SimonNyvall
Copy link
Contributor Author

Thank you for the feedback! Made two more commits addressing the issues.

@SimonNyvall SimonNyvall requested a review from ardalis November 28, 2023 13:16
@ardalis ardalis merged commit 4fa5c69 into ardalis:main Dec 11, 2023
1 check failed
@ardalis
Copy link
Owner

ardalis commented Dec 11, 2023

Thanks, I'll get this out soon!

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.

2 participants