-
Notifications
You must be signed in to change notification settings - Fork 173
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
feat(java): Add errorprone to Java sourcesets #451
Conversation
Looks like one error on:
|
Should have added this comment here instead of the PR on adding detekt for Kotlin... Looks like some of the crashes we're running into are the same ones noted in google/error-prone#1195, though that PR has been open for a year. |
Yeah, I found that PR yesterday too. I think Google's Java team is pretty anti-Lombok in general, so I don't think they're particularly motivated to merge workarounds for its weird behavior... |
I kinda feel like as long as Lombok is floating around, SCA in Java is just going to be a pain and will inevitably result in us ignoring all the rules as Lombok spreads use. So, there's really only a couple options I can see (feel free to propose others):
In the case of the first option, I'd be even more inclined to push the project towards deeper use of Kotlin, since we have no technical barriers with SCA in Kotlin, not to mention the language itself prevents a lot of things errorprone provides for Java. In the case of the second option, we'd have to migrate service-by-service with probably a big-bang refactor to rid of Lombok, and then a followup to apply SCA and fix the things it has initially raised. |
I've bumped heads with Lombok a bunch because it's pretty inflexible compared to auto value, but I fear an uprising if we forbid its use. Creating an auto value data class is absolutely more work. Converting everything by hand is probably too huge to actually accomplish (and almost certain to introduce errors, since we can't rely on the Groovy compiler to let us know when we accidentally change an API), so I guess you're talking about just replacing the Lombok annotations with the generated code? I suppose I personally would be in favor of doing that (maybe just until I see what it actually looks like), but not strongly enough to push hard for it. Here's a list of lombok import counts to show what we're up against:
|
I actually think that Lombok makes it too easy to add too many getters (and worse setters)...I think it has encouraged us to just add That being said, migrating away from Lombok does seem like a pretty big undertaking, and might not be the highest priority, so I'm not overly inclined to push forward with it now. If we want to push to clean up a lot of tech debt, I'd advocate finally getting rid of all of our Groovy, which would then make changes like this a lot safer going forward. |
I agree it'd be too much of an undertaking. So we continue getting rid of Groovy. Do we push people towards vanilla Java, Lombok'd Java, or Kotlin? If we do Lombok'd Java, we're only making our future lives worse if we decide to get rid of it. I mean, I'd be ok with not spreading Lombok use purely because of its ergonomics with Jackson. 😆 |
I'd push towards Kotlin, personally. But with the huge caveat that you can't mix Kotlin and Groovy in the same compilation unit, which is really limiting since there's still so much Groovy left. |
Closing. |
And here's the Java version. I guess since Lombok does a bunch of crazy stuff with internal javac APIs, errorprone can be a little dicey. I've disabled the checks that cause kork to blow up.
Just wanted this out there so people can debate errorprone + Lombok specifically.