-
Notifications
You must be signed in to change notification settings - Fork 253
Refactor the state of order expressions as OneOrMany. #509
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the handling of order expressions by transitioning from a List to a OneOrMany data structure, aiming to simplify and standardize the state management of order expressions. Key changes include:
- Renaming unit tests to more accurately reflect the type of order chain (OrderBy vs. OrderByDescending) and adding a new test for handling invalid root chains.
- Updating the Specification class to use OneOrMany<OrderExpressionInfo> instead of a List.
- Modifying the OrderEvaluator logic to use new OneOrMany order expression properties and to enhance duplicate order chain detection.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
tests/Ardalis.Specification.Tests/Evaluators/OrderEvaluatorTests.cs | Updated unit tests to reflect new ordering constraints and exception expectations. |
src/Ardalis.Specification/Specification.cs | Refactored order expressions to utilize OneOrMany and updated cloning/adding logic accordingly. |
src/Ardalis.Specification/Evaluators/OrderEvaluator.cs | Modified evaluator logic to process OneOrMany order expressions and adjusted duplicate order chain validation. |
Comments suppressed due to low confidence (1)
src/Ardalis.Specification/Specification.cs:194
- Verify that the Clone method on OneOrMany properly creates a deep copy as needed to avoid unintended shared state between specifications.
otherSpec._orderExpressions = _orderExpressions.Clone();
It addresses the issue #506