Clean up synthetic accessor method rot #2762
Conversation
c00d6d4 to
71c806a
Compare
|
Hah I pushed a branch that migrated to 6.0.0 recently as well, though with no thought given to refactoring in general: https://github.com/sjudd/glide/tree/pmd_6_0_0 |
sjudd
left a comment
There was a problem hiding this comment.
Looks better than my changes, I can try to rebase on top of it to get to 6.0.0+.
Just a couple of questions.
| return into(target, /*targetListener=*/ null); | ||
| } | ||
|
|
||
| @RestrictTo(Scope.LIBRARY) |
There was a problem hiding this comment.
I'm ok not using this for non-public methods. I assume anything that's non-public can be broken and even some of the public APIs we have are breakable (those really should be marked with @RestrictTo or some equivalent.
Fine to keep it, just as a note for the future (or a discussion point).
There was a problem hiding this comment.
Lot of negation there :)
You're right: default methods don't need this, if someone uses "internal" (name/keyword for default visibility in other languages) API, they can fix it if we break something. protected on public classes may need this, and public methods that are only public because of packages, and not because it's public API also need this. I'll remove from default ones.
| @SuppressWarnings("WeakerAccess") | ||
| @Synthetic | ||
| final DecodeHelper<R> decodeHelper = new DecodeHelper<>(); | ||
| private final DecodeHelper<R> decodeHelper = new DecodeHelper<>(); |
|
|
||
| public Key get(int size) { | ||
| Key result = get(); | ||
| Key result = super.get(); |
There was a problem hiding this comment.
Out of curiosity, what does the super do?
There was a problem hiding this comment.
Open the file on master, there's a warning on it. It clarifies which get is being called: both BaseKeyPool and SizeStrategy have a get name in scope, so the super clears it up which one. Actually let me check that again, because it's a static inner class.
There was a problem hiding this comment.
so the circumstances I described above stand, but it may be an inspection false positive as the inner class is static and the get signature is different in the outer class. I guess it still plays nicer with the super..
There was a problem hiding this comment.
…l) methods should already be protected because users need to explicitly put accessor code into Glide packages to call the methods.
|
@sjudd merged master into this, resolved conflict, removed |
|
Thanks! I'll add the files you suggest to the gitignore, no particular reason to exclude them I don't think. |
Description
Since PMD 5.5.4 there's a
AccessorMethodGenerationrule indesigncategory thanks to Jake's request. The PR removes 20 synthetic methods and prevents adding any new ones.In this PR:
@Syntheticannotation for future ci build failuresAvoidDuplicateLiteralsand simplifiedUncommentedEmptyConstructormaven-metadata.xmldownloads.In some files (
ActiveResources,DecodeJob,ViewTarget.OnAttachStateChangeListener, ) I took a different approach than making everything visible. It looks like in those classes the inner classes had some field envy to the outer class, so I moved the method to the outer class so there's only one visible@Syntheticentry needed. In some places I also protected the published methods with@RestrictTo(Scope.LIBRARY)to flag abuse.Motivation and Context
Followup to #1556 (comment)
After this PR only
disklrucachesynthetic accessors (22) remain.To check count, run
@sjudd Can you please add
!.idea/inspectionProfiles/Project_Default.xmlto.gitignoreand commit that file so everyone sees the same warnings in AS? I wanted to addJava/J2ME issues/Synthetic accessor callinspection to the config, but I noticed that file is not version controlled.