Skip to content

Conversation

@timtebeek
Copy link
Contributor

We've defined some recipe best practices at OpenRewrite to ensure recipes look and behave the same, and avoid some potential problems: https://docs.openrewrite.org/recipes/recipes/rewrite/openrewriterecipebestpractices

I've applied these best practices here already, and added the plugin to make them easy to run again. It would also be possible to run these best practices on pull requests and get suggestions for improvements there. Let me know if that is of interest.

Copy link
Contributor

Copilot AI left a 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 applies OpenRewrite recipe best practices to the codebase, including code formatting improvements, parameter naming consistency, and IDE language injection hints for better developer experience. The changes also add the OpenRewrite Maven plugin configuration to make it easier to run these best practices in the future.

Key Changes

  • Added OpenRewrite Maven plugin to enable automated best practice enforcement
  • Improved test code readability by converting string concatenation to text blocks with @Language("java") annotations
  • Standardized parameter naming in visitor methods from executionContext to ctx
  • Simplified conditional logic by removing unnecessary else blocks and using early returns
  • Applied string literal comparison best practices using .equals() with literals on the left side

Reviewed Changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated no comments.

Show a summary per file
File Description
migration/pom.xml Added OpenRewrite Maven plugin configuration with rewrite-migrate-java and rewrite-rewrite dependencies
SortingMigrationRecipeTest.java Added @language annotations and converted to helper methods with language hints
SolverManagerBuilderRecipeTest.java Added @language annotations, reordered imports, and added //language=java comment
SolutionManagerRecommendAssignmentRecipeTest.java Added @language annotations to helper methods
SingleConstraintAssertionRecipeTest.java Added @language annotations to helper methods
ScoreManagerMethodsRecipeTest.java Added @language annotations to helper methods
ScoreGettersRecipeTest.java Added @language annotations, converted string concatenation to text blocks
RemoveConstraintPackageRecipeTest.java Converted string concatenation to text blocks with @language annotations
NullableRecipeTest.java Added @language annotations and //language=java comments
ConstraintRefRecipeTest.java Added @language annotations to helper methods
AsConstraintRecipeTest.java Converted extensive string concatenation to text blocks, added @language annotations
SolverManagerBuilderRecipe.java Renamed executionContext parameter to ctx
ScoreManagerMethodsRecipe.java Renamed executionContext parameter to ctx, fixed string formatting
ScoreGettersRecipe.java Renamed executionContext parameter to ctx, simplified if-else logic, reformatted string concatenation
ConstraintRefRecipe.java Renamed executionContext parameter to ctx
AsConstraintRecipe.java Renamed executionContext parameter to ctx, simplified nested if-else logic, applied string comparison best practices
TimefoldChangeDependencies.java Applied string comparison best practices

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@timtebeek timtebeek marked this pull request as draft November 17, 2025 17:19
@timtebeek
Copy link
Contributor Author

Ok there's quite a few checks on the builds here that I have yet to work out; to start with spotless doesn't apply for me, which is annoying, and then it seems I have to manage the dependency of a new jetbrains annotations dependency. Would you even want that added here before I put more effort in?

@triceo
Copy link
Collaborator

triceo commented Nov 17, 2025

Hello @timtebeek!
Thank you for the PR - I'd avoid the IntelliJ dependency.
The other dependencies can be managed in build-parent POM.
Wrt. spotless - mvn clean install should do the job, does for me every single time.

@timtebeek
Copy link
Contributor Author

Pulling out some surprising likely unrelated failures from the logs:

[INFO] Running ai.timefold.solver.enterprise.solver.quarkus.TimefoldEnterpriseProcessorWithoutNodeSharingSolveTest
Error:  Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.867 s <<< FAILURE! -- in ai.timefold.solver.enterprise.solver.quarkus.TimefoldEnterpriseProcessorWithoutNodeSharingSolveTest
Error:  ai.timefold.solver.enterprise.solver.quarkus.TimefoldEnterpriseProcessorWithoutNodeSharingSolveTest -- Time elapsed: 0.867 s <<< ERROR!
java.lang.ExceptionInInitializerError
	at io.quarkus.runtime.generated.StaticInitConfigCustomizer.configBuilder(Unknown Source)
	at io.smallrye.config.SmallRyeConfigBuilder.build(SmallRyeConfigBuilder.java:772)
	at io.quarkus.runtime.generated.Config.<clinit>(Unknown Source)
	at io.quarkus.runner.ApplicationImpl.<clinit>(Unknown Source)
	at java.base/java.lang.Class.forName0(Native Method)
	at java.base/java.lang.Class.forName(Class.java:469)
	at io.quarkus.runner.bootstrap.StartupActionImpl.run(StartupActionImpl.java:334)
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:705)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	Suppressed: java.lang.NoClassDefFoundError: Could not initialize class io.quarkus.runtime.generated.Config
		at java.base/java.lang.Class.forName0(Native Method)
		at java.base/java.lang.Class.forName(Class.java:469)
		at io.quarkus.runner.bootstrap.StartupActionImpl.run(StartupActionImpl.java:338)
		... 2 more
	Caused by: java.lang.ExceptionInInitializerError: Exception java.lang.ExceptionInInitializerError [in thread "main"]
		at io.quarkus.runtime.generated.StaticInitConfigCustomizer.configBuilder(Unknown Source)
		at io.smallrye.config.SmallRyeConfigBuilder.build(SmallRyeConfigBuilder.java:772)
		at io.quarkus.runtime.generated.Config.<clinit>(Unknown Source)
		at io.quarkus.runner.ApplicationImpl.<clinit>(Unknown Source)
		at java.base/java.lang.Class.forName0(Native Method)
		at java.base/java.lang.Class.forName(Class.java:469)
		at io.quarkus.runner.bootstrap.StartupActionImpl.run(StartupActionImpl.java:334)
		... 2 more
Caused by: java.lang.reflect.UndeclaredThrowableException
	at io.smallrye.config.ConfigMappingLoader.configMappingSecrets(ConfigMappingLoader.java:107)
	at io.smallrye.config.ConfigMappings$ConfigClass.<init>(ConfigMappings.java:127)
	at io.smallrye.config.ConfigMappings$ConfigClass.configClass(ConfigMappings.java:188)
	at io.quarkus.runtime.configuration.AbstractConfigBuilder.configClass(AbstractConfigBuilder.java:133)
	at io.quarkus.runtime.generated.SharedConfig.<clinit>(Unknown Source)
	... 9 more
Caused by: java.lang.NoSuchMethodError: no such method: ai.timefold.solver.quarkus.config.TimefoldRuntimeConfig$$CMImpl.getSecrets()Set/invokeStatic
	at io.smallrye.config.ConfigMappingLoader$ConfigMappingImplementation.<init>(ConfigMappingLoader.java:211)
	at io.smallrye.config.ConfigMappingLoader$1.computeValue(ConfigMappingLoader.java:29)
	at io.smallrye.config.ConfigMappingLoader$1.computeValue(ConfigMappingLoader.java:26)
	at java.base/java.lang.ClassValue.getFromHashMap(ClassValue.java:228)
	at java.base/java.lang.ClassValue.getFromBackup(ClassValue.java:210)
	at java.base/java.lang.ClassValue.get(ClassValue.java:116)
	at io.smallrye.config.ConfigMappingLoader.configMappingSecrets(ConfigMappingLoader.java:91)
	... 13 more

[INFO] 
[INFO] Results:
[INFO] 
Error:  Errors: 
Error:    TimefoldEnterpriseProcessorNodeSharingSolveTest » ExceptionInInitializer
Error:    TimefoldEnterpriseProcessorWithoutNodeSharingSolveTest » ExceptionInInitializer

@triceo
Copy link
Collaborator

triceo commented Nov 17, 2025

@timtebeek I think this one will go away if you rebase to latest main. (In the meantime, a Quarkus version update happened on main and downstream, and it resulted in a mismatch with your branch.)

@timtebeek
Copy link
Contributor Author

Great, thanks for the help! Should be ok to review now. :)

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.

Reviewed and pushed minor changes.

I decided against including the POM changes. We try to inherit as much of OpenRewrite from the BOM, but this upgrade recipe is not part of that. This would have unfortunate consequences, specifically:

  • Dependabot creates PRs automatically on all dependency updates.
  • OpenRewrite already triggers two of them every time you release a new version. (One for the BOM, another for the plugin.) This would have introduced a third.
  • And this happens frequently enough as to cause churn, which I'd like to keep to a minimum.

However, I see the benefit of these refactors made here; once (if) the recipe becomes part of the BOM, I will be happy to use it.

@sonarqubecloud
Copy link

@timtebeek
Copy link
Contributor Author

Thanks for the review! Fine indeed to drop the plugin; it's not necessary to add but does make it easier to find. The same recipe can also be run using this command without changes to the build file:

mvn -U org.openrewrite.maven:rewrite-maven-plugin:run -Drewrite.recipeArtifactCoordinates=org.openrewrite.recipe:rewrite-rewrite:RELEASE -Drewrite.activeRecipes=org.openrewrite.recipes.rewrite.OpenRewriteRecipeBestPractices -Drewrite.exportDatatables=true

@triceo triceo merged commit 78a2f52 into TimefoldAI:main Nov 18, 2025
34 checks passed
@triceo
Copy link
Collaborator

triceo commented Nov 18, 2025

Thank you, @timtebeek!

@timtebeek timtebeek deleted the apply-openrewrite-recipe-best-practices branch November 18, 2025 11:19
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.

2 participants