Skip to content
This repository has been archived by the owner on Nov 3, 2024. It is now read-only.

Only delete getters that have the canonical form #726

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rillig
Copy link
Contributor

@rillig rillig commented Jan 18, 2020

#688

This is just a proof of concept, to get some early feedback.

Of course the same work would have to be done for the setters as well.

And maybe for the equals and hashCode methods. It might get trickier to test whether these are of the few established standard forms, as produced by Eclipse, IntelliJ and a few other IDEs.

My main focus are the getters and setters for now since manually checking for typos in that code is absolutely boring and error-prone.

While testing I noticed that Lombok will add a getter for fieldWithoutGetter. This may be intentional or not.

@codecov-io
Copy link

codecov-io commented Jan 18, 2020

Codecov Report

Merging #726 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #726      +/-   ##
============================================
- Coverage      69.7%   69.69%   -0.02%     
  Complexity     1778     1778              
============================================
  Files           200      200              
  Lines          6173     6173              
  Branches       1266     1266              
============================================
- Hits           4303     4302       -1     
- Misses         1261     1262       +1     
  Partials        609      609
Impacted Files Coverage Δ Complexity Δ
...llij/plugin/action/lombok/LombokGetterHandler.java 100% <100%> (+12.5%) 4 <0> (+1) ⬆️
.../intellij/plugin/psi/LombokLightMethodBuilder.java 42.37% <0%> (-1.7%) 26% <0%> (-1%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b63c99...1389451. Read the comment docs.

@rillig rillig requested a review from mplushnikov February 14, 2020 18:03
@rillig
Copy link
Contributor Author

rillig commented Feb 14, 2020

@mplushnikov Do you have an opinion on the code I have written? If you like it, I would implement similar code and tests for the setters as well.

@mplushnikov
Copy link
Owner

Hello @rillig

thank you very much for your contribution. I'll look on your PR next days!

Please add yourself to contributors list in readme.md file, we forget to do it previously.

Copy link
Owner

@mplushnikov mplushnikov left a comment

Choose a reason for hiding this comment

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

Hello @rillig !
Sorry for my very late answer. I looked now on your 'proof of concept' and it looks good!
We can try to change implementation this way, it should work for @Getter and @Setter I think. Maybe it is necessary to add some checks/tests for inheritance, to check field and method are from same class for example. I think your current implementation with findGetterForField looks into super classes too.

'Getter'-generation for fieldWithoutGetter field is not intentional of course. Just didn't see this bug before. It's maybe not very critical, and is just ignoring by people. Correct implementation should add @Getter(AccessLevel.NONE) to it i think.

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

Successfully merging this pull request may close these issues.

3 participants