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

Don't include SUGGESTION checks in default checks #1380

Merged
merged 3 commits into from
Jun 2, 2020

Conversation

pkoenig10
Copy link
Member

@pkoenig10 pkoenig10 commented Jun 2, 2020

When new SUGGESTION checks are added, it adds noise to the compiler output.

Even if these checks have suggested fixes, there is nothing that enforces SUGGESTION checks. So subsequent changes can introduce regressions that clutter the compiler output.

As a result I've found myself adding the config like this to all our repos to ensure that we enforce the default patch checks when new SUGGESTION checks are added.

options.errorprone.errorproneArgs += [
    '-Xep:AssertjRefactoring:WARN',
    '-Xep:CatchSpecificity:WARN',
    '-Xep:LambdaMethodReference:WARN',
    '-Xep:PreferAssertj:WARN',
    '-Xep:PreferCollectionConstructors:WARN',
    '-Xep:PreferBuiltInConcurrentKeySet:WARN',
    '-Xep:RedundantMethodReference:WARN',
    '-Xep:RedundantModifier:WARN',
    '-Xep:StreamOfEmpty:WARN',
    '-Xep:ThrowSpecificity:WARN']

Relying on excavator upgrades to fix these results in code churn and doesn't seem like the right approach. I'd would strongly prefer to either:

  1. Enforce these checks at the warning level so projects can opt-in to failing at compile time for all new checks using -Werror.
  2. Don't include SUGGESTION checks in default checks.

This PR takes approach (1), but I'm indifferent between these two options (or some mix of them depending on the check).

@changelog-app
Copy link

changelog-app bot commented Jun 2, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

The baseline-error-prone plugin no longer applies SUGGESTION checks by default.

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from iamdanfox June 2, 2020 21:26
Copy link
Contributor

@ferozco ferozco left a comment

Choose a reason for hiding this comment

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

Overall supportive of making these checks (especially since most autofix) warning/errors

@@ -41,7 +42,7 @@
link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
linkType = BugPattern.LinkType.CUSTOM,
providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION,
severity = BugPattern.SeverityLevel.SUGGESTION,
severity = SeverityLevel.WARNING,
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like this should be an error

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want WARN here, iirc we use a flag to ignore error-prone warnings in generated code but it doesn't apply to errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we explicitly opt out of errorprone on generated source sets:

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we change this in a FLUP? I'd prefer not to accidentally break things in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for handling warn->error separately. I don't think our regex catches 100% of generated code and I'm not sure it's worth the risk breaking some folks in order to fail builds in repos that don't fail on errors

@@ -33,7 +34,7 @@
name = "StreamOfEmpty",
link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
linkType = BugPattern.LinkType.CUSTOM,
severity = BugPattern.SeverityLevel.SUGGESTION,
severity = SeverityLevel.WARNING,
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, definitely a bug

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the risk with making some of these errors is that repos which have a bunch of existing violation will be blocked. WARNING seems a bit safer because it's won't fail compilation unless you have -Werror.

Happy to change it if you would still prefer ERROR, just wanted to point this out.

Copy link
Contributor

Choose a reason for hiding this comment

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

in practice these rules will be auto applied on upgrade so I'm not particularly worried about blocking upgrades.

@ferozco
Copy link
Contributor

ferozco commented Jun 2, 2020

👍

@bulldozer-bot bulldozer-bot bot merged commit d48df4c into develop Jun 2, 2020
@bulldozer-bot bulldozer-bot bot deleted the pkoenig10/suggestion branch June 2, 2020 22:34
@svc-autorelease
Copy link
Collaborator

Released 3.20.0

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

4 participants