-
Notifications
You must be signed in to change notification settings - Fork 440
TEZ-4642: Introduce spotless plugin and enforce basic import styles #423
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
LOL, yetus thinks that the patch is not applicable, I'm afraid we need to merge and test afterwards with a dummy precommit |
|
@abstractdog we can merge it and test after raising dummy PR to master branch. Samething we followed for spotbugs related PRs aswell :) |
ayushtkn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, minor suggestion.
This gonna make backports a big pain, if this iteself isn't backported
pom.xml
Outdated
| <plugin> | ||
| <groupId>com.diffplug.spotless</groupId> | ||
| <artifactId>spotless-maven-plugin</artifactId> | ||
| <version>2.43.0</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you define a variable for the version and use it here, like other dependencies. Moreover why not use the latest version:
https://mvnrepository.com/artifact/com.diffplug.spotless/spotless-maven-plugin/2.46.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
This comment was marked as outdated.
This comment was marked as outdated.
|
💔 -1 overall
This message was automatically generated. |
|
Ohh, I remember this Yetus issue. The size of the PR is too big that is why it is failing to apply. If you do locally on your mac This will fail locally as well More details here: Maybe we can hack it here as well, get the result and then revert that hacky commit then merge this PR Can you apply this diff and trigger the build please |
nice! let me try |
|
seems to be working, in progress: |
This comment was marked as outdated.
This comment was marked as outdated.
|
🎊 +1 overall
This message was automatically generated. |
|
looks clear, thanks @ayushtkn! reverted the workaround commit |
|
💔 -1 overall
This message was automatically generated. |
|
@ayushtkn: let me know if we can merge this |
ayushtkn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Huge patch that reorganize imports. Easy to implement and port:
Import Ordering Enforcement
Configured to match a reasonable standard: java,javax,org.apache,com,net,io
Automatically removes unused imports
Separates import groups with blank lines
Build Integration
Fails builds when import violations are found (during validate phase)
Works with all Maven goals: install, package, compile, etc.