Conversation
triceo
requested review from
Christopher-Chianelli,
TomCools and
zepfred
as code owners
There was a problem hiding this comment.
Pull request overview
This PR contains miscellaneous "leftover" changes that address various technical debt items and API modernizations. The changes are intentionally unrelated and will not be squashed, as noted in the PR description.
Changes:
- Standardized locale format from underscore (
cs_cz) to hyphen (cs-CZ) notation usingLocale.forLanguageTag()andtoLanguageTag() - Removed the
ProblemId_generic type parameter fromSolverManager,SolverJob,SolverJobBuilderand related classes, simplifying the API to useObjectfor problem IDs - Updated Jackson API calls from deprecated methods (
serializationInclusion(),getCurrentName(),fields()) to modern equivalents (defaultPropertyInclusion(),currentName(),properties()) - Updated JUnit 5 API to add
ParameterDeclarationsparameter toArgumentsProvider.provideArguments() - Modernized Gizmo API calls from
objEquals()toexprEquals() - Modernized thread factory implementation using Java's
Thread.ofPlatform()builder pattern - Added strict validation for constraint names and groups (alphanumeric, spaces, underscores, hyphens, apostrophes, dots only)
- Migrated from
@NonNullto@NullMarkedwith@Nullablefor null safety annotations - Added no-args constructor to
MutableReferenceclass - Simplified compiler warnings configuration to
-Xlint:none
Reviewed changes
Copilot reviewed 80 out of 80 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| locale XML/Java files | Changed locale format from underscore to hyphen notation (cs_cz → cs-CZ) |
| SolverManager and related | Removed ProblemId_ generic parameter, simplified to Object |
| Jackson integration | Updated deprecated Jackson APIs to modern equivalents |
| TerminationTest.java | Added ParameterDeclarations parameter for JUnit 5 compatibility |
| Thread factory | Modernized using Thread.ofPlatform() builder |
| AbstractConstraintBuilder | Added constraint name/group validation with tests |
| Build configuration | Simplified compiler args to suppress all warnings |
| Gizmo classes | Updated from objEquals to exprEquals |
| Null annotations | Migrated to @NullMarked pattern throughout |
| var trimmed = fieldValue.trim(); | ||
| if (trimmed.isEmpty()) { | ||
| throw new IllegalArgumentException( | ||
| "The %s (%s) must not be empty or only contain whitespace.".formatted(fieldName, fieldName)); |
There was a problem hiding this comment.
The error message at line 55 duplicates fieldName instead of fieldValue. It should format fieldValue, not fieldName twice.
| "The %s (%s) must not be empty or only contain whitespace.".formatted(fieldName, fieldName)); | |
| "The %s (%s) must not be empty or only contain whitespace.".formatted(fieldName, fieldValue)); |
| if (sanitized.isEmpty()) { | ||
| throw new IllegalArgumentException("The %s cannot be empty." | ||
| .formatted("constraint name")); |
There was a problem hiding this comment.
The redundant check at line 28 is unnecessary since sanitize() at line 27 already validates non-empty strings. The sanitized value can never be empty at this point.
| <!-- | ||
| Suppress warnings about generated code to reduce log length. | ||
| Warnings are primarily dealt with during development in the IDE. | ||
| --> | ||
| <compilerArgument>-Xlint:none</compilerArgument> |
There was a problem hiding this comment.
The compiler configuration disables all warnings with -Xlint:none, but the comment suggests warnings should be handled in the IDE. However, this approach may allow warnings to accumulate in the codebase since they won't be visible during CI builds. Consider using a more targeted approach that suppresses only specific unavoidable warnings while keeping critical warnings enabled.
| - [ ] `Rebaser` renamed to `Lookup` and made public. | ||
| - [ ] `ProblemChangeDirector#lookUpWorkingObject()` no longer returns `Optional`. | ||
| - [ ] `AutoDiscoverMemberType` is gone | ||
| - [ ] Constraint name and group now forces strict validation. |
There was a problem hiding this comment.
The removal of the ProblemId_ generic type parameter from SolverManager, SolverJob, and related classes is a significant API breaking change that should be documented in docs/TODO.md. Users will need to remove the second type parameter from their code, which is a non-trivial migration.
|
1 participant
The commits will not be squashed, they are too unrelated.