Skip to content

Conversation

zepfred
Copy link
Contributor

@zepfred zepfred commented Sep 25, 2025

No description provided.

@zepfred
Copy link
Contributor Author

zepfred commented Sep 25, 2025

@triceo I noticed other factories used by configurations and annotations that are not located in the public API. Maybe, we should handle that in a separate PR as well.

Copy link
Collaborator

@triceo triceo left a comment

Choose a reason for hiding this comment

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

As always, the different selector combinations are causing me to ask questions - see inline.

Downstream tests are failing.

Also, please provide a migration recipe for the move of the interface to the public API; the uses of the old interface should be replaced by the new one.

Finally, the change needs to show up in the upgrade recipe in our docs.

@zepfred
Copy link
Contributor Author

zepfred commented Sep 26, 2025

Finally, the change needs to show up in the upgrade recipe in our docs.

I have included a recipe for automatic migration to the new interface. Do we need to add anything to our docs?

@zepfred
Copy link
Contributor Author

zepfred commented Oct 14, 2025

@triceo we include the property sorterWeightFactoryClass in EntitySelectorConfig, ValueSelectorConfig, and MoveSelectorConfig. Should we deprecate it and introduce a new property called sorterComparatorFactoryClass?

The property sorterWeightFactoryClass is already of type ComparatorFactory.

@triceo
Copy link
Collaborator

triceo commented Oct 14, 2025

@triceo we include the property sorterWeightFactoryClass in EntitySelectorConfig, ValueSelectorConfig, and MoveSelectorConfig. Should we deprecate it and introduce a new property called sorterComparatorFactoryClass?

Yes, the old names should be deprecated. Code which uses them should still work, but the code which combines the old names and the new names should fail fast - it's either all the old names, or all the new names.

Arguably sorterComparatorFactoryClass - why not just ComparatorFactoryClass? I'm not sure.

@zepfred
Copy link
Contributor Author

zepfred commented Oct 14, 2025

@triceo we include the property sorterWeightFactoryClass in EntitySelectorConfig, ValueSelectorConfig, and MoveSelectorConfig. Should we deprecate it and introduce a new property called sorterComparatorFactoryClass?

Yes, the old names should be deprecated. Code which uses them should still work, but the code which combines the old names and the new names should fail fast - it's either all the old names, or all the new names.

Arguably sorterComparatorFactoryClass - why not just ComparatorFactoryClass? I'm not sure.

We already have sorterComparatorClass of type Comparator

Copy link

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.

Construction Heuristic full configuration for a PlanningListVariable

2 participants