Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature(LambdaBlockToExpression): convert lambda with method invocation as well #241

Conversation

timo-abele
Copy link
Contributor

@timo-abele timo-abele commented Jan 16, 2024

What's changed?

This PR will enable the transformation of lambdas with method invocation as body.

What's your motivation?

Fixes #236

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

  1. The first test case I added doesn't work, see comment in code
  2. I had to explicitly set the whitespace in the return statement (LambdaBlockToExpression.java L61) in order to have the code move up to the previous line. Is there a cleaner way to do that?

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

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 self-requested a review January 16, 2024 19:07
@timtebeek timtebeek added the enhancement New feature or request label Jan 16, 2024
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Great start at contributing here @timo-abele ! I've added a comment to explain the use of classpaths for tests, and suggestion to remove the second test. Beyond that I think we can try this out if you agree with removing that second test.

the other test covers the expected behavior sufficiently

Co-authored-by: Tim te Beek <timtebeek@gmail.com>
@timo-abele
Copy link
Contributor Author

timo-abele commented Jan 16, 2024

There was another test that failed namely UseLambdaForFunctionalInterfaceTest::nestedLambdaInMethodArgument I didn't expect this but apparently UseLambdaForFunctionalInterface depends on this recipe here. Log:

expected: 
  "import java.util.function.Consumer;
  
  class Test {
      void bar(Consumer<Integer> c) {
      }
      void foo() {
          bar(i -> {
              bar(i2 -> {
              });
          });
      }
  }"
 but was: 
  "import java.util.function.Consumer;
  
  class Test {
      void bar(Consumer<Integer> c) {
      }
      void foo() {
          bar(i -> bar(i2 -> {
          }));
      }
  }"

The new result looks good to me, can I simply adapt UseLambdaForFunctionalInterfaceTest::nestedLambdaInMethodArgument?

@timtebeek
Copy link
Contributor

The new result looks good to me, can I simply adapt UseLambdaForFunctionalInterfaceTest::nestedLambdaInMethodArgument?

Yeah sure! Agree with your observation there. Feel free to mark your PR ready for review once done.

@timo-abele timo-abele marked this pull request as ready for review January 16, 2024 22:35
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Added two small changes to minimize casts and white space changes; hope you agree! Typically we leave white space changes to the specific style configured rather than setting an explicit space to use. Figured in this case folks might have chosen to add white space, rather than { statement }, so we should be mindful an keep that in.

Neat addition to this recipe! Look forward to how it will affect changes at scale. Thanks again!

@timtebeek timtebeek merged commit f1e53d2 into openrewrite:main Jan 16, 2024
1 check passed
@timo-abele
Copy link
Contributor Author

I agree with the casts and I don't mind the white space changes, it did feel strange adding it explicitly in the code. But the reason I did it was to be consistent with the existing behavior

void simplifyLambdaBlockToExpression() {
rewriteRun(
//language=java
java(
"""
import java.util.function.Function;
class Test {
Function<Integer, Integer> f = n -> {
return n+1;
};
}
""",
"""
import java.util.function.Function;
class Test {
Function<Integer, Integer> f = n -> n+1;
}
"""
)
);
}

where the white space is removed (because the expression is extracted from the statement and the statement holding the white space is dropped). Since the statement in the lambda is only one line, I think it is less of a surprise to continue the line with the -> and not to break it (by keeping the white space).

@timtebeek
Copy link
Contributor

You're right; in the case of return we implicitly drop any white space that was associated with the return by only returning the expression. Guess we'll have to see how this works out in practice if we should do the same for statements as well. I do quite like how only the lines that contained { and } are changed with the current approach leaving the line with the statement untouched. If you used { statement } on a single line it also works as expected already. To be seen when run at scale. :)

@timo-abele
Copy link
Contributor Author

Hi @timtebeek, could you please release a new version? I'd like to try these changes out.

@timtebeek
Copy link
Contributor

Know that you can always try out the latest builds from our snapshot versions; we'll likely folllow up with a release next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

LambdaBlockToExpression does not work in assertThrows (or generally when no value is returned)
2 participants