-
Notifications
You must be signed in to change notification settings - Fork 178
chore: fail-fast validation for invalid value ranges #2069
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 pull request implements fail-fast validation for invalid value ranges to catch configuration errors early when planning values are assigned outside their defined value ranges.
Changes:
- Added value range validation that checks all assigned planning values are within their respective value ranges
- Validation runs once at solver initialization and optionally during move execution in FULL_ASSERT mode
- Comprehensive test coverage for basic variables, list variables, multi-variable entities, and custom phases
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/main/java/ai/timefold/solver/core/impl/score/director/InnerScoreDirector.java | Added interface method for value range assertion |
| core/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirector.java | Implemented value range validation logic for basic and list variables, integrated into move execution assertions |
| core/src/main/java/ai/timefold/solver/core/impl/solver/scope/SolverScope.java | Added one-time value range check during initial solution setup |
| core/src/main/java/ai/timefold/solver/core/impl/phase/custom/DefaultCustomPhase.java | Added value range assertion for custom phases when running in FULL_ASSERT mode |
| core/src/test/java/ai/timefold/solver/core/impl/solver/DefaultSolverTest.java | Added comprehensive tests for value range validation across different variable types and scenarios |
core/src/main/java/ai/timefold/solver/core/impl/score/director/InnerScoreDirector.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/solver/scope/SolverScope.java
Outdated
Show resolved
Hide resolved
core/src/test/java/ai/timefold/solver/core/impl/solver/DefaultSolverTest.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/solver/scope/SolverScope.java
Outdated
Show resolved
Hide resolved
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
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
core/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirector.java
Outdated
Show resolved
Hide resolved
triceo
left a comment
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.
Much better! IMO still room for improvement.
core/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirector.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirector.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirector.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirector.java
Outdated
Show resolved
Hide resolved
...a/ai/timefold/solver/core/testdomain/clone/customcloner/TestdataCorrectlyClonedSolution.java
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirector.java
Outdated
Show resolved
Hide resolved
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
Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.
core/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirector.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirector.java
Show resolved
Hide resolved
|
Having just merged my other PR, this one now has conflicts. :-( |
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
Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.
|



No description provided.