search

openrewrite / rewrite-analysis

OpenRewrite recipes for data flow analysis.
Apache License 2.0
8 stars 8 forks source link

refactor: OpenRewrite best practices #46

Closed timtebeek closed 6 months ago

timtebeek commented 6 months ago

Use this link to re-run the recipe: https://app.moderne.io/recipes/org.openrewrite.recipes.OpenRewriteBestPractices?organizationId=T3BlblJld3JpdGU%3D

JLLeitschuh commented 6 months ago

Some of those @DocumentExample don't really make sense IMHO

timtebeek commented 6 months ago

To contextualize this a bit: this is the output of a recipe run where we select the first non-issue test that asserts a change is made. Typically that's the first test in the test classes, to serve as an example of how the recipe can be used. I did do a quick scan before I merged, but of course don't know all the ins and outs of this project. If you feel some can be moved/improved I'd love to hear.

The reason I ran this recipe is such that we can automatically review PRs going forward, where we can limit the PR review feedback to just newly added recipes and tests. Folks will get a suggestion on PRs to pick an example if they had not already; such that going forward we pick examples for new additions, as well as a number of other best practices.

There's always a challenge when applying changes at scale how much you go in depth to verify a recipe output. In this case the heuristic if perhaps optimistic, and might indeed have picked an example where you might have picked a different one.

JLLeitschuh commented 6 months ago

I can understand that, but the visitors being tested only exist in tests. They don't exist in production code. I recognize what the annotation is supposed to be for, but in this project, the API is where the value is, not necessarily any single visitor or recipe.