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

Improvement: Disable errorprone in intellij #2010

Merged
merged 3 commits into from
Dec 7, 2021

Conversation

ferozco
Copy link
Contributor

@ferozco ferozco commented Dec 7, 2021

Before this PR

Running test/compilation from intellij with gradle integration was pretty painful since it would cause all errorprone rules to run. This slowed down overall compilation but also made it hard for you to quickly iterate on a test, or hack together a proof of concept

After this PR

==COMMIT_MSG==
Disable errorprone in intellij
==COMMIT_MSG==

This will not impact CI in any way and should only make the gradle integration's behaviour closer to working with a *ipr file

cc @iamdanfox

Possible downsides?

@policy-bot policy-bot bot requested a review from carterkozak December 7, 2021 14:42
@iamdanfox
Copy link
Contributor

iamdanfox commented Dec 7, 2021

I think this would probably be net positive, but just trying to consider possible downsides:

  • I think we would no longer get cache hits from compilation inside IntelliJ vs in a terminal/on CI
  • a few people might actually want to see all the ErrorProne rules as squiggly lines in their IDE 🤷 ??

One alternative might be downgrade things from error->warning or even info in IntelliJ?

public final class IntellijSupport {
private static final String INTELLIJ_ACTIVE = "idea.active";

public static boolean isRunningInIntellij() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind consolidating the idea.active checks from BaselineIdea as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course! done

@carterkozak
Copy link
Contributor

a few people might actually want to see all the ErrorProne rules as squiggly lines in their IDE shrug ??

Does this actually happen? I didn't think idea picked up error-prone warnings, but most of the projects I work on fail in that case, so I may not have seen it. If so, I'd be down for opting out of -Werror when intellij gradle integration is used.

Re caching: It's unfortunate to lose gradle caching, but it's a bit difficult to prototype things with StrictUnusedVariable enabled, so I think the trade-off makes sense.

@iamdanfox
Copy link
Contributor

Yeah StrictUnusedVariable is savage. 👍 from me.

@ferozco
Copy link
Contributor Author

ferozco commented Dec 7, 2021

I don't believe that intellij shows squiggly for errorprone OOTB

Copy link
Contributor

@carterkozak carterkozak left a comment

Choose a reason for hiding this comment

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

tyvm!

@bulldozer-bot bulldozer-bot bot merged commit 1918d07 into develop Dec 7, 2021
@bulldozer-bot bulldozer-bot bot deleted the fo/disable-errorprone-in-intellij branch December 7, 2021 15:14
@svc-autorelease
Copy link
Collaborator

Released 4.48.0

bulldozer-bot bot pushed a commit to palantir/witchcraft-api that referenced this pull request Dec 8, 2021
###### _excavator_ is a bot for automating changes across repositories.

Changes produced by the roomba/latest-baseline-oss check.

# Release Notes
## 4.47.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Fix | Gradle plugins don't enforce PublicConstructorForAbstractClass which can break gradle injection | palantir/gradle-baseline#2009 |


## 4.48.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Improvement | Disable errorprone in intellij | palantir/gradle-baseline#2010 |


## 4.49.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Improvement | Allow projects to override library auto-detection | palantir/gradle-baseline#2011 |


## 4.50.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Improvement | Improve coordination between java-versions and idea ipr allowing project iprs to be successfully imported | palantir/gradle-baseline#2012 |



To enable or disable this check, please contact the maintainers of Excavator.
This was referenced Dec 8, 2021
@pkoenig10
Copy link
Member

pkoenig10 commented Jan 13, 2022

While I understand the motivation for this change, it breaks what is, IMO, one of the biggest benefits of using the native Gradle integration - shared build infrastructure and caching.

When switching between my IDE and the command line, I now need to compile everything twice. In my experience, being able to avoid expensive recompilations is far more valuable that avoiding error-prone warnings when iterating.

@pkoenig10
Copy link
Member

pkoenig10 commented Jan 13, 2022

I'm also not convinced that disabling error-prone is the correct tradeoff. It can be very confusing if my code doesn't pass CI due to an error I cannot see when I build locally. It now becomes very time-consuming to get passing CI build, especially if I have errors in multiple projects that depend on each other transitively because I will only see one error at a time. Avoiding this requires knowing that compiling on the command line will show these errors.

I would much rather know that my code won't pass CI before I push (since I'm almost certainly compiling locally anyways) instead of pushing thinking it's good, and then finding out 5-10 minutes later than it was not good.

@pkoenig10
Copy link
Member

pkoenig10 commented Jan 13, 2022

At the very least, I'd appreciate if we gave devs an option to avoid this "feature". AFAICT there's no way for me to prevent IntelliJ from setting the idea.active property in my builds, so I'm stuck with this behavior.

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

Successfully merging this pull request may close these issues.

None yet

6 participants