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

Convert multi param lambdas and local method invocations to method references #1365

Merged
merged 9 commits into from
Jun 1, 2020

Conversation

ferozco
Copy link
Contributor

@ferozco ferozco commented May 29, 2020

Before this PR

We did not catch or convert any lambda that took multiple parameters to method references and we did not convert any local instance method either

After this PR

==COMMIT_MSG==
Convert multi param lambdas and local method invocations to method references
==COMMIT_MSG==

Possible downsides?

We want to make sure this works in all cases so we should test against our large internal repo before merging

@ferozco ferozco requested a review from carterkozak May 29, 2020 03:45
@changelog-app
Copy link

changelog-app bot commented May 29, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Convert multi param lambdas and local method invokations to method references

Check the box to generate changelog(s)

  • Generate changelog entry

@ferozco ferozco changed the title Convert multi param lambdas and local method invokations to method references Convert multi param lambdas and local method invocations to method references May 29, 2020
Copy link
Contributor

@carterkozak carterkozak left a comment

Choose a reason for hiding this comment

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

Looks slick!

" return value.length();",
" }",
"}")
.doTest();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure these negative tests are necessary when we have refactoring tests as well.

VisitorState state,
boolean isLocal) {
if (!symbol.isStatic() && isLocal) {
return "this." + symbol.name.toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there might be some oddness if the method is. It provided by this, but an enclosing class, requiring prefix “Enclosing.this.“. Worth a test to verify, I’d probably exclude that from the check rather than attempt to refactor since it’s fairly uncommon.


ExpressionTree receiver = ASTHelpers.getReceiver(methodInvocation);
boolean isLocal = isLocal(methodInvocation);
if (!isLocal && !(receiver instanceof IdentifierTree)) {
Copy link
Contributor

@carterkozak carterkozak May 29, 2020

Choose a reason for hiding this comment

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

Does this prevent us from fixing static method calls? I don't think static methods have receivers (but I may be incorrect)

- value -> Preconditions.checkNotNull(value)
+ Preconditions::checkNotNull

@carterkozak
Copy link
Contributor

Seeing a lot of places where we're not qualifying the method reference correctly. This results in:

-        open().runVoid(conn -> consumer.accept(conn));
+        open().runVoid(Consumer::accept);

where we want:

-        open().runVoid(conn -> consumer.accept(conn));
+        open().runVoid(consumer::accept);

however I think we only want to do this when consumer is final or effectively final. In the original the value of consumer was read when the funciton is called, where in the new version we capture the reference when the lambda is created.

forozco and others added 3 commits May 29, 2020 15:32
* Fix lambdas

* dont change reference capture time

* avoid refactoring lambdas with param types

* cyclo
<!-- User-facing outcomes this PR delivers -->
Copy link
Contributor

@carterkozak carterkozak left a comment

Choose a reason for hiding this comment

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

Validated on a large internal project

@ferozco
Copy link
Contributor Author

ferozco commented Jun 1, 2020

👍

@bulldozer-bot bulldozer-bot bot merged commit 5740878 into develop Jun 1, 2020
@bulldozer-bot bulldozer-bot bot deleted the fo/lambda-methods branch June 1, 2020 20:55
@svc-autorelease
Copy link
Collaborator

Released 3.19.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants