refactor: ScoreDirector is no longer public by triceo · Pull Request #2129 · TimefoldAI/timefold-solver

triceo · 2026-02-19T08:09:30Z

No description provided.

Copilot

Pull request overview

This pull request refactors the ScoreDirector from public API to internal implementation as part of Timefold Solver 2.0. The changes involve moving ScoreDirector from ai.timefold.solver.core.api.score.director to ai.timefold.solver.core.impl.score.director, and introducing a new PhaseCommandContext interface to replace the direct use of ScoreDirector in the public API. This is a significant breaking change that improves API encapsulation.

Changes:

ScoreDirector moved from public API to internal implementation package
PhaseCommand interface refactored to use PhaseCommandContext instead of ScoreDirector and BooleanSupplier parameters
SolutionPartitioner.splitWorkingSolution() signature changed to accept Solution_ instead of ScoreDirector<Solution_>
Documentation updated to reflect new API patterns and remove deprecated content
Test domain classes updated (comparator/factory naming consistency improvements)
MoveDirector enhanced with new assignValuesAndAdd method and executeTemporary variants

Reviewed changes

Copilot reviewed 123 out of 123 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
core/src/main/java/ai/timefold/solver/core/impl/score/director/ScoreDirector.java	Moved from api package to impl package, marked as non-public API
core/src/main/java/ai/timefold/solver/core/api/solver/phase/PhaseCommand.java	Interface refactored to use PhaseCommandContext parameter
core/src/main/java/ai/timefold/solver/core/api/solver/phase/PhaseCommandContext.java	New interface providing access to working solution and move execution
core/src/main/java/ai/timefold/solver/core/impl/phase/custom/DefaultPhaseCommandContext.java	Implementation of PhaseCommandContext
core/src/main/java/ai/timefold/solver/core/impl/partitionedsearch/partitioner/SolutionPartitioner.java	Interface updated to remove ScoreDirector dependency
core/src/main/java/ai/timefold/solver/core/impl/move/MoveDirector.java	Enhanced with new assignValuesAndAdd method and executeTemporary variants
docs/src/modules/ROOT/pages/optimization-algorithms/overview.adoc	Updated PhaseCommand documentation with new API
docs/src/modules/ROOT/pages/responding-to-change/responding-to-change.adoc	Removed deprecated PinningFilter section, updated ProblemChange references
docs/src/modules/ROOT/pages/constraints-and-score/constraint-configuration.adoc	Removed deprecated constraint configuration section
Multiple test files	Updated PhaseCommand implementations to use new API, comparator/factory naming improvements
Multiple impl files	Updated imports from api.score.director to impl.score.director

docs/src/modules/ROOT/pages/optimization-algorithms/overview.adoc

core/src/main/java/ai/timefold/solver/core/api/domain/solution/PlanningEntityProperty.java

core/src/main/java/ai/timefold/solver/core/impl/move/MoveDirector.java

core/src/main/java/ai/timefold/solver/core/impl/move/AbstractMove.java

Copilot

Pull request overview

Copilot reviewed 166 out of 166 changed files in this pull request and generated 2 comments.

core/src/test/java/ai/timefold/solver/core/impl/solver/SolverMetricsIT.java

core/src/main/java/ai/timefold/solver/core/api/solver/phase/PhaseCommandContext.java

sonarqubecloud · 2026-02-19T09:47:38Z

Quality Gate passed

Issues
17 New issues
0 Accepted issues

Measures
0 Security Hotspots
75.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Christopher-Chianelli

I dislike renaming Rebaser to Lookup; the methods are more awkward, and I believe some changes should be made; in particular, the lookup/rebase method should not have "return null if does not exist" method, and instead I propose these two methods:

@NullMarked
interface Rebaser {
    @Nullable
    <Type_> rebase(@Nullable Type_ toRebase);
    <Type_> rebaseExisting(Type_ toRebase);
}

Both methods fail fast if toRebase does not exist in the ScoreDirector. They only differ in one accepts null (and thus return a nullable), the other does not (and thus always return non-null).

Christopher-Chianelli · 2026-02-19T14:49:26Z

core/src/main/java/ai/timefold/solver/core/api/solver/phase/PhaseCommandContext.java

+     * As defined by {@link #execute(Move, boolean)},
+     * but with the guarantee of a fresh score.
+     */
+    default void execute(Move<Solution_> move) {


I think the execute commands should return the score (that way, users don't need to go through getWorkingSolution).

Christopher-Chianelli · 2026-02-19T14:57:07Z

core/src/main/java/ai/timefold/solver/core/impl/domain/entity/descriptor/EntityDescriptor.java

-        isInitializedPredicate = this::isInitialized;
        if (entityClass.getPackage() == null) {
-            LOGGER.warn("The entityClass ({}) should be in a proper java package.", entityClass);
+            LOGGER.warn("The entityClass ({}) is in an unnamed package.", entityClass);


Why do we warn about this?

Christopher-Chianelli · 2026-02-19T15:36:47Z

...ld/solver/core/impl/heuristic/selector/move/generic/list/kopt/SelectorBasedKOptListMove.java


    @Override
-    public SelectorBasedKOptListMove<Solution_> rebase(Rebaser rebaser) {
+    public SelectorBasedKOptListMove<Solution_> rebase(Lookup lookup) {


I dislike replacing Rebaser with Lookup. First, when rebasing, you always want to fail-fast (whereas with Lookup, you might want to return null if the entity does not exist for problem changes). Second, the method name is long and awkward; rebase is much nicer.

Christopher-Chianelli · 2026-02-19T15:39:16Z

...imefold/solver/core/impl/heuristic/selector/move/generic/list/SelectorBasedListSwapMove.java

+    public SelectorBasedListSwapMove<Solution_> rebase(Lookup lookup) {
+        return new SelectorBasedListSwapMove<>(variableDescriptor,
+                Objects.requireNonNull(lookup.lookUpWorkingObjectOrFail(leftEntity)),
+                leftIndex, Objects.requireNonNull(lookup.lookUpWorkingObjectOrFail(rightEntity)), rightIndex);


Objects.requireNonNull is worthless, since lookUpWorkingObjectOrFail will fail-fast if there no corresponding object, and leftEntity/rightEntity are guarantee to not be null.

Christopher-Chianelli · 2026-02-19T15:39:54Z

...efold/solver/core/impl/heuristic/selector/move/generic/list/SelectorBasedListChangeMove.java

-                sourceIndex, Objects.requireNonNull(rebaser.rebase(destinationEntity)), destinationIndex);
+    public SelectorBasedListChangeMove<Solution_> rebase(Lookup lookup) {
+        return new SelectorBasedListChangeMove<>(variableDescriptor,
+                Objects.requireNonNull(lookup.lookUpWorkingObjectOrFail(sourceEntity)),


Christopher-Chianelli · 2026-02-19T15:55:28Z

core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/ListAssignMove.java

-        return new ListAssignMove<>(variableMetaModel, Objects.requireNonNull(rebaser.rebase(planningValue)),
-                Objects.requireNonNull(rebaser.rebase(destinationEntity)), destinationIndex);
+    public Move<Solution_> rebase(Lookup lookup) {
+        return new ListAssignMove<>(variableMetaModel, Objects.requireNonNull(lookup.lookUpWorkingObjectOrFail(planningValue)),


Christopher-Chianelli · 2026-02-19T15:55:45Z

core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/ListChangeMove.java

        return new ListChangeMove<>(variableMetaModel,
-                Objects.requireNonNull(rebaser.rebase(sourceEntity)), sourceIndex,
-                Objects.requireNonNull(rebaser.rebase(destinationEntity)), destinationIndex);
+                Objects.requireNonNull(lookup.lookUpWorkingObjectOrFail(sourceEntity)), sourceIndex,


Christopher-Chianelli · 2026-02-19T15:57:16Z

core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/ListUnassignMove.java

-    public Move<Solution_> rebase(Rebaser rebaser) {
-        return new ListUnassignMove<>(variableMetaModel, Objects.requireNonNull(rebaser.rebase(sourceEntity)), sourceIndex);
+    public Move<Solution_> rebase(Lookup lookup) {
+        return new ListUnassignMove<>(variableMetaModel, Objects.requireNonNull(lookup.lookUpWorkingObjectOrFail(sourceEntity)),


Christopher-Chianelli · 2026-02-19T16:05:56Z

...i/timefold/solver/core/impl/constructionheuristic/DefaultConstructionHeuristicPhaseTest.java

                assertThat(entity.getValueList()).hasSize(1);
-                assertThat(TestdataObjectSortableDescendingFactory.extractCode(entity.getValueList().get(0).getCode()))
+                assertThat(
+                        TestdataObjectSortableDescendingComparator.extractCode(entity.getValueList().get(0).getCode()))


Suggested change

TestdataObjectSortableDescendingComparator.extractCode(entity.getValueList().get(0).getCode()))

TestdataObjectSortableDescendingComparator.extractCode(entity.getValueList().getFirst().getCode()))

Christopher-Chianelli · 2026-02-19T16:16:22Z

core/src/test/java/ai/timefold/solver/core/impl/move/MoveDirectorTest.java

+        }
+
+        @Test
+        void assignValuesAndAddToEmptyList() {


Are these duplicates of the tests above?

triceo added 2 commits February 19, 2026 08:55

refactor: no score director in custom phase

5bfbc84

refactor: score director no longer public

6ca559f

Copilot AI review requested due to automatic review settings February 19, 2026 08:09

triceo requested review from Christopher-Chianelli, TomCools and zepfred as code owners February 19, 2026 08:09

triceo temporarily deployed to internal February 19, 2026 08:09 — with GitHub Actions Inactive

triceo deployed to documentation (preview) February 19, 2026 08:10 — with GitHub Actions View deployment

Copilot started reviewing on behalf of triceo February 19, 2026 08:10 View session

triceo added this to the v2.0.0.CR1 milestone Feb 19, 2026

Copilot AI reviewed Feb 19, 2026

View reviewed changes

triceo force-pushed the scoredirector branch from f3948c7 to 903c988 Compare February 19, 2026 08:32

triceo temporarily deployed to internal February 19, 2026 08:32 — with GitHub Actions Inactive

triceo deployed to documentation (preview) February 19, 2026 08:32 — with GitHub Actions View deployment

Copilot AI review requested due to automatic review settings February 19, 2026 09:16

triceo force-pushed the scoredirector branch from 903c988 to 012ee2e Compare February 19, 2026 09:16

triceo temporarily deployed to internal February 19, 2026 09:16 — with GitHub Actions Inactive

triceo deployed to documentation (preview) February 19, 2026 09:17 — with GitHub Actions View deployment

Copilot started reviewing on behalf of triceo February 19, 2026 09:17 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

core/src/test/java/ai/timefold/solver/core/impl/solver/SolverMetricsIT.java Outdated Show resolved Hide resolved

core/src/main/java/ai/timefold/solver/core/api/solver/phase/PhaseCommandContext.java Outdated Show resolved Hide resolved

refactor: remove all leftover deprecations

b1a2c0b

triceo force-pushed the scoredirector branch from 012ee2e to b1a2c0b Compare February 19, 2026 09:30

triceo temporarily deployed to internal February 19, 2026 09:30 — with GitHub Actions Inactive

triceo deployed to documentation (preview) February 19, 2026 09:30 — with GitHub Actions View deployment

Christopher-Chianelli requested changes Feb 19, 2026

View reviewed changes

	TestdataObjectSortableDescendingComparator.extractCode(entity.getValueList().get(0).getCode()))
	TestdataObjectSortableDescendingComparator.extractCode(entity.getValueList().getFirst().getCode()))

Conversation

triceo commented Feb 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Feb 19, 2026

Quality Gate passed

Uh oh!

Christopher-Chianelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments