Skip to content

Conversation

e5LA
Copy link
Contributor

@e5LA e5LA commented Jul 21, 2025

What's changed?

Added new FluentSetterRecipe to convert void setter methods (and optionally other void methods) into fluent-style methods that return this.

What's your motivation?

Anything in particular you'd like reviewers to focus on?

  • Indentation and placement of the injected return this; statement.
  • Return types use the simple name (e.g. Inner instead of Outer.Inner)

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@timtebeek timtebeek moved this from In Progress to Ready to Review in OpenRewrite Jul 21, 2025
Copy link
Contributor

@greg-at-moderne greg-at-moderne left a comment

Choose a reason for hiding this comment

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

The indentation and inner classes look fine.

Having seen your run example, for both the default mode and the includeAllVoidMethods as well - I think a check should be added whether the method is not an override of some super method. In this case, I think it's probably wrong to change the return type. And now, I am not sure how feasible this check would be in all cases. Wdyt?

@e5LA
Copy link
Contributor Author

e5LA commented Jul 23, 2025

Thanks for the review @greg-at-moderne - that's good point, I will have a look.

github-actions[bot]

This comment was marked as outdated.

@e5LA
Copy link
Contributor Author

e5LA commented Jul 30, 2025

Sorry for the delay, I didn't have much time lately.
While working on detecting whether a method overrides a superclass method, I realized there's a case we need to consider:

public class Parent {
    public void setName(String name) {
        this.name = name;
    }
}

public class Child extends Parent {
    private String name;

    @Override
    public void setName(String name) {
        this.name = name;
    }
}

In this situation, I see two options:

  1. Either transform both Parent#setName and Child#setName to return this (+changing the return type) - that means we're explicitly transforming overridden methods.

  2. Or skip transforming Child#setName because it’s an override - that seems straightforward.
    However, the tricky part is with Parent#setName - it's not itself an override, but it is overridden.
    I wonder - can we reliably detect that it’s being overridden in a subclass?

@timtebeek
Copy link
Member

Thanks both! Wondering a bit indeed how we can make this safe; we don't immediately have a way to know if a method might be overridden, unless we use a ScanningRecipe, and even then we might miss cases where it's a library. Perhaps the safe thing to do would be to limit this to either the provided method pattern (opt in), or final classes when left out.

We can optionally and separately expand FinalClass to make it a scanning recipe that adds that modifier to any classes not extended, that way the combination of these two recipes would achieve the safe results we're after.

@e5LA
Copy link
Contributor Author

e5LA commented Sep 1, 2025

Thanks for the feedback @timtebeek, that's good suggestion.

I've updated the recipe, now by default it converts methods only in final classes. When the pattern is provided, it converts methods in any class.

Your idea about expanding FinalClass recipe also sounds good. I might take a look at it too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Ready to Review
Development

Successfully merging this pull request may close these issues.

Make setters return this to allow for fluent interfaces
3 participants