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

236 var for methods #249

Merged
merged 10 commits into from
Jul 10, 2023
Merged

236 var for methods #249

merged 10 commits into from
Jul 10, 2023

Conversation

MBoegers
Copy link
Contributor

@MBoegers MBoegers commented Jun 29, 2023

What's changed?

Implemented Issue #236.

What's your motivation?

Make Open Rewrite var aware

Any additional context

Method invocation without generics were already implemented by UseVarForObject and are only proved by additional tests.

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've added the license header to any new files through ./gradlew licenseFormat
  • I've used the IntelliJ auto-formatter on affected files
  • I've updated the documentation (if applicable)

@MBoegers MBoegers closed this Jun 29, 2023
@MBoegers MBoegers force-pushed the 236-var_for_methods branch from bbd0a13 to 927b01d Compare June 29, 2023 12:28
@MBoegers MBoegers reopened this Jun 29, 2023
@knutwannheden
Copy link
Contributor

@MBoegers Great to see you making progress here! Please ping us if you have any questions or would like us to review it. While it is still in draft, we will just let you continue working.

@MBoegers
Copy link
Contributor Author

MBoegers commented Jul 7, 2023

@MBoegers Great to see you making progress here! Please ping us if you have any questions or would like us to review it. While it is still in draft, we will just let you continue working.

Before undrafting I will implement the missing UseVarForMethodInvocations. But you could take a look on UseVarForGenericsConstructors and the tests

MBoegers added 2 commits July 10, 2023 09:42
* Update name of UseVarForGenericMethodInvocations to reflect action
* Fix yml recipe java-lang-var.yml
* update tests to reflect shortcomings of open rewrite regarding generic method invocations
@MBoegers MBoegers marked this pull request as ready for review July 10, 2023 07:58
@timtebeek timtebeek requested a review from joanvr July 10, 2023 09:01
Copy link
Contributor

@joanvr joanvr left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution. Couple of very small comments. Let me know if you want me to merge this PR already :)

class A {
void m() {
List<Object> os = new List();
List<Object> os = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Good improvement :) Our parser might be a bit too permissive sometimes :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH, there is warning "com/example/app/A.java:7: error: List is abstract; cannot be instantiated List os = new List<>();" but I think that should be an error.

}
@Test
void withDiamondOperatorOnRaw() {
//todo check if this may be possible!, We could transform ArrayList into ArrayList<String>
Copy link
Contributor

Choose a reason for hiding this comment

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

I've quickly tried to see the changes needed for this, and I think it's ok to leave it as future improvement since it requires quite not so trivial changes..

@@ -17,11 +17,14 @@
type: specs.openrewrite.org/v1beta/recipe
name: org.openrewrite.java.migrate.lang.UseVar
displayName: Use local variable type inference
description: Apply local variable type inference (`var`) for primitives and objects.
description: Apply local variable type inference (`var`) for primitives and objects. These recipes can cause unused
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Co-authored-by: Joan Viladrosa <[email protected]>
@MBoegers
Copy link
Contributor Author

Thank you very much for your contribution. Couple of very small comments. Let me know if you want me to merge this PR already :)

Thanks for your review. I've accepted your suggestions, after the CI succeeded I am ok with merging.

@joanvr joanvr merged commit 1a50a7b into openrewrite:main Jul 10, 2023
@joanvr
Copy link
Contributor

joanvr commented Jul 10, 2023

Merged it, thank you very much again for your contribution, and thanks for the fixes on the previous recipes and tests :)

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

Successfully merging this pull request may close these issues.

3 participants