Skip to content

Update spotless to 7.1.0 #18770

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Pankraz76
Copy link

@Pankraz76 Pankraz76 commented Jul 16, 2025

Update spotless to 7.1.0

[Describe what this change achieves]

while experimenting with rewrite i came across this custom solution for what recently has been integrated into spot.

could not do it without @iddeepak, thanks for the decent impl.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@Pankraz76 Pankraz76 requested a review from a team as a code owner July 16, 2025 15:11
@Pankraz76 Pankraz76 force-pushed the use-removeWildcardImports branch from 246ec7e to 3c3d1c9 Compare July 16, 2025 15:12
@Pankraz76 Pankraz76 changed the title use spotless removeWildcardImports for custom implementation use spotless instead of custom implementation for removeWildcardImports Jul 16, 2025
@Pankraz76 Pankraz76 changed the title use spotless instead of custom implementation for removeWildcardImports use spotless implementation of removeWildcardImports Jul 16, 2025
@Pankraz76 Pankraz76 changed the title use spotless implementation of removeWildcardImports Fix spotless implementation of removeWildcardImports Jul 16, 2025
@Pankraz76 Pankraz76 force-pushed the use-removeWildcardImports branch from 3c3d1c9 to 2ea7836 Compare July 16, 2025 15:23
Copy link
Contributor

❌ Gradle check result for 2ea7836: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@Pankraz76

This comment was marked as resolved.

@owaiskazi19
Copy link
Member

Looks like the support was added recently diffplug/spotless#2517, we relied on the custom logic until now

@Pankraz76 Pankraz76 changed the title Fix spotless implementation of removeWildcardImports Update spotless to 7.1.0 Jul 16, 2025
Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

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

Thanks @Pankraz76 for the change!

@Pankraz76 Pankraz76 force-pushed the use-removeWildcardImports branch from 965ef23 to ad24bc0 Compare July 16, 2025 19:17
@Pankraz76
Copy link
Author

Thanks @Pankraz76 for the change!

could not do it without @iddeepak, thanks for the decent impl.

@Pankraz76
Copy link
Author

Pankraz76 commented Jul 16, 2025

Looks like the support was added recently diffplug/spotless#2517, we relied on the custom logic until now

kindly FYI: its been around, so might be any good, despite the academical risk. @sbrannen

@Pankraz76 Pankraz76 force-pushed the use-removeWildcardImports branch from ad24bc0 to bd777ca Compare July 16, 2025 19:27
@owaiskazi19
Copy link
Member

owaiskazi19 commented Jul 16, 2025

@Pankraz76 I tested this on local, with this plugin it removes the wildcard imports directly and doesn't add associated imports. After running spotlessApply imports are removed, it causes compile issues.
Whereas the current implementation calls out the file where wildcard import was used

Step 'Refuse wildcard imports' found problem in 'src/test/java/org/opensearch/neuralsearch/processor/normalization/ZScoreNormalizationTechniqueTests.java':
Do not use wildcard imports.  'spotlessApply' cannot resolve this issue.
java.lang.AssertionError: Do not use wildcard imports.  'spotlessApply' cannot resolve this issue.
        at java.base/jdk.internal.reflect.DirectConstructorHandleAccessor.newInstance(DirectConstructorHandleAccessor.java:62)
        at formatting_a046l3dushzjseo478wpr27to$_run_closure1$_closure2$_closure3$_closure6.doCall$original(/Users/kazabdu/Documents/neural-search/gradle/formatting.gradle:17)
        at formatting_a046l3dushzjseo478wpr27to$_run_closure1$_closure2$_closure3$_closure6.doCall(/Users/kazabdu/Documents/neural-search/gradle/formatting.gradle)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)

@Pankraz76
Copy link
Author

yes its just able to remove not replace unfortunately. Assuming its just a simple regex as before its not the workaround solution provided by issue discussion, moved into dedicated rule.

@Pankraz76
Copy link
Author

yes its just able to remove not replace unfortunately. Assuming its just a simple regex as before its not the workaround solution provided by issue discussion, moved into dedicated rule.

its assuming that removal of the code is not compiling anymore therefore unable to ship.

The local changes can then be resolved by some IDE just opening the changed files.

As spot has no type awareness its not able to really replace the wildcard like desired and expected as spot normally delivers the fixes.

Rewrite can deliver this.

But we can and should adapt to fail the build and not break the code. This is kind of awkward. Just throwing the error as before on wildcard detected would be more complain, with very good reasoning given by @sbrannen

Copy link
Contributor

❌ Gradle check result for bd777ca: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for bd777ca: ABORTED

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@iddeepak
Copy link

I agree with the proposal that failing the build with a clear lint error is safer and more predictable than silently removing critical imports. Thank you @Pankraz76, Learning as always from your contributions!

@owaiskazi19
Copy link
Member

@Pankraz76 should we wait for diffplug/spotless#2557 to released with the new version and then we can make the changes to use removeWildcardImports?

@Pankraz76
Copy link
Author

sure thing no need to rush.

Actually should just dedicate the feature in PR. Is does not change anything, as it seems here are not wildcards.
Having one thing only, is always nice approach. Thanks for suggestion.

Signed-off-by: Vincent Potucek <[email protected]>
@Pankraz76 Pankraz76 force-pushed the use-removeWildcardImports branch from bd777ca to f95f1d9 Compare July 17, 2025 20:26
@Pankraz76 Pankraz76 requested a review from owaiskazi19 July 17, 2025 20:27
Copy link
Contributor

❌ Gradle check result for f95f1d9: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

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.

4 participants