openrewrite / rewrite-static-analysis

OpenRewrite recipes for identifying and fixing static analysis issues.
Apache License 2.0
27 stars 43 forks source link

UseAsBuilder needs a clearer description #263

Closed timo-abele closed 2 months ago

timo-abele commented 4 months ago

After looking at the tests, the description makes sense, but initially I was hoping that this recipe could turn (actual lombok) setter calls into a builder call chain, so for:

@Setter
class A {
    private int foo;
    private int ba;
}

before:

A a = new A();
a.setFoo(1);
a.setBa(2);

after:

A a = A.builder().foo(1).ba(2);

I think "series of setter calls" is what tripped me up and let me think that a builder would be added where so far no builder had been. I suggest something like "chain builder calls" and, if possible a code snippet.

Are you interested in contributing a fix to OpenRewrite?

timtebeek commented 4 months ago

Good feedback, thanks! Looking at the recipe these are the current displayName and description. https://github.com/openrewrite/rewrite-static-analysis/blob/6fc920533401e0f9a098fde655bc648d4dfb4087/src/main/java/org/openrewrite/staticanalysis/UseAsBuilder.java#L59-L65

Whereas indeed the test is clearer about the before/after https://github.com/openrewrite/rewrite-static-analysis/blob/6fc920533401e0f9a098fde655bc648d4dfb4087/src/test/java/org/openrewrite/staticanalysis/UseAsBuilderTest.java#L50-L75

We'd previously shown the @DocumentExample in the description, which could be a way to surface more clearly what a recipe does

Until then, it's probably best to change the name and description to cover what would help prevent confusion. Any suggestions?

timo-abele commented 2 months ago

I thought there was a recipe that chains AssertJ asserts from separate lines that I could use for inspiration, but I can no longer find it 🫤 -> feature request I would suggest: Class Name: ChainCallsToBuilderMethods Recipe Name: Chain calls to builder methods on separate lines into one chain of builder calls. Recipe Description: When an API has been designed as a builder, join method invocations into one call chain Feel free to adjust, I will not have the time to make a PR for this.

timtebeek commented 2 months ago

I thought there was a recipe that chains AssertJ asserts from separate lines that I could use for inspiration, but I can no longer find it 🫤

You might have seen this PR previously, that still needs a more detailed review & merge. You can already use if for inspiration though, as from the tests it does seem to work. A thing I haven't been able to fit in so far is very that it does not make any unintended changes, which is why review has taken some time.

feature request I would suggest: Class Name: ChainCallsToBuilderMethods Recipe Name: Chain calls to builder methods on separate lines into one chain of builder calls. Recipe Description: When an API has been designed as a builder, join method invocations into one call chain Feel free to adjust, I will not have the time to make a PR for this.

Those are good suggestions already, thanks! @mike-solomon would you be able to update these descriptions? You might have some additional thoughts on the phrasing to prevent confusion as to how this ends up in the docs.