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

Implement automatic fixes for ImmutablesStyle #1846

Merged
merged 3 commits into from
Jul 23, 2021

Conversation

carterkozak
Copy link
Contributor

Before this PR

Lots of blocked baseline-upgrade excavators, which in turn block Gradle 7 rollout.

After this PR

Autofix the ImmutablesStyle check in a naive way. I'm not thrilled with the code we produce, but I think it satisfies the classpath cleanliness concern without requiring manual intervention.
==COMMIT_MSG==
Implement automatic fixes for ImmutablesStyle
==COMMIT_MSG==

Possible downsides?

The error-prone fix api doesn't allow us to create new files, so I've added suppressions for our OuterTypeFilename and OneTopLevelClass checkstyle rules.
Ideally we'd detect style configurations that match our style library and use that instead.
Ideally we'd aggregate identical style configurations into a single meta-annotation instead of creating one per class.

@changelog-app
Copy link

changelog-app bot commented Jul 23, 2021

Generate changelog in changelog/@unreleased

Type

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

Description

Implement automatic fixes for ImmutablesStyle

Check the box to generate changelog(s)

  • Generate changelog entry

@pkoenig10
Copy link
Member

Another option is to just fix by suppressing warnings for this check.

This check is really targeted at libraries - it matters less for servers. And most of our widely used libraries are well maintained and not blocked on this check.

The check will still prevent new occurrences because we don't apply fixes by default, so that seems like a reasonable compromise. Especially given that this check has been out for almost 2 months now.

@carterkozak
Copy link
Contributor Author

That's fair, I've updated the top-level-class case to suppress, while nested classes still get a new meta-annotation

" @Target(ElementType.TYPE)",
" @Retention(RetentionPolicy.SOURCE)",
" @Value.Style(visibility = Value.Style.ImplementationVisibility.PUBLIC)",
" @interface PersonStyle {}",
Copy link
Member

Choose a reason for hiding this comment

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

Can this be private? Or does that not work with immutables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would fail to compile when encapsulated within an interface, in that case the annotation will be public. I’m not terribly worried about that case because it won’t have any runtime impact, only build time.

@bulldozer-bot bulldozer-bot bot merged commit 23343da into develop Jul 23, 2021
@bulldozer-bot bulldozer-bot bot deleted the ckozak/fix_immutablestyle branch July 23, 2021 14:26
@svc-autorelease
Copy link
Collaborator

Released 4.5.0

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

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

# Release Notes
## 4.5.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Improvement | Implement automatic fixes for `ImmutablesStyle` | palantir/gradle-baseline#1846 |


## 4.6.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Improvement | Suppress existing `ProxyNonConstantType` failures to ease rollout | palantir/gradle-baseline#1850 |



To enable or disable this check, please contact the maintainers of Excavator.
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