Update code from the internal repo. #1
Conversation
* This open sources the PluginCleaner checker * Batch pulls a number of changes from our internal repo * Gets us generally ready to making Piranha OSS the source of truth overall (including internally).
|
I did my best to check for references to internal stuff and add the licenses (and the tests run :) ). But a second pair of eyes is always helpful, @mkruber |
Done. |
| */ | ||
| package com.uber.piranha; | ||
|
|
||
| public class PiranhaUtils { |
There was a problem hiding this comment.
This class also needs to be deleted as it is used for internally diff processing.
There was a problem hiding this comment.
That's not exactly practical, though. This class is used by Piranha to mark code for file deletion, so we need to reference it from the EP check. We are not (yet?) providing the scripts to remove files marked with these comments, but we probably should.
We could make the comment text a parameter to Piranha (maybe in a follow up PR?), but I am not sure that would be any cleaner than this.
There was a problem hiding this comment.
providing a script to delete the files with this comment addresses my concern.
There was a problem hiding this comment.
Ok, will do in a follow up diff, opening an issue for now.
| /** | ||
| * Originally from com.uber.errorprone.checker.dagger.UsageCounter | ||
| */ | ||
| public final class UsageCounter { |
There was a problem hiding this comment.
EP was already planning on a usage checker for Dagger. However, no harm in releasing this.
There was a problem hiding this comment.
They do have a checker, but it's not implemented with a modular usage counter we can easily call, for now it's better to keep this class around, if not the corresponding UsageChecker :)
| private boolean countsCollected = false; | ||
| private ImmutableMap<Symbol, UsageCounter.CounterData> usageCounts = null; | ||
| private Map<Symbol, Integer> deletedUsages = null; | ||
|
|
||
| /** | ||
| * Copied from NullAway comment. Error Prone requires us to have an empty constructor for each |
There was a problem hiding this comment.
This line can be deleted :)
There was a problem hiding this comment.
Seems useful for documenting why we have that constructor, though. I can remove the reference from NullAway if you want, but the comment itself seems useful.
| } | ||
|
|
||
| @Override | ||
| public Description matchClass(ClassTree classTree, VisitorState visitorState) { |
There was a problem hiding this comment.
this will work well internally, not sure how this is appropriate for external use. We need to think of a better flow here.
There was a problem hiding this comment.
I think I agree, although I am also not sure the external flow needs to be too different (except for using gradle instead of buck and integrations with whatever the users' CI is). For now, I think moving the development to the open can be done first and then we can improve the UX for external adopters :)
| private BugCheckerRefactoringTestHelper addHelperClasses(BugCheckerRefactoringTestHelper bcr) | ||
| throws IOException { | ||
| return bcr.addInputLines( | ||
| "XPTest.java", |
There was a problem hiding this comment.
Can we place them as part of the refactoring files? It is hard to parse them manually and also putting them as part of the refactoring files will ensure consistency and list the respective use cases.
overall (including internally).