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

Apply checkstyle rule UnusedImports #34477

Conversation

pankratz76
Copy link

I saw the rules are not fully aligned. Assuming the minimum approach will be shared, such as avoiding used ballast as already approached.

  • Sync checkstyle rules
  • Apply UnusedImports

PS: This is just a suggestion to serve as a basis for discussion—no need to merge this all at once in a big bang. I can also split it up according to priorities (separation of concerns).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 24, 2025
@pankratz76 pankratz76 force-pushed the UnusedLocalVariable-fix-UnusedImports branch 2 times, most recently from b6a0cbd to 1509c5e Compare February 24, 2025 15:22
Signed-off-by: Vincent Potucek <[email protected]>
@pankratz76 pankratz76 force-pushed the UnusedLocalVariable-fix-UnusedImports branch from 1509c5e to 9135e97 Compare February 24, 2025 15:24
@pankratz76
Copy link
Author

is this any good?

@pankratz76 pankratz76 marked this pull request as ready for review February 24, 2025 15:25
@bclozel
Copy link
Member

bclozel commented Feb 24, 2025

I think this PR should stick to its original intent: apply checkstyle rules for imports to the main sourceset.

Can you please add the "AvoidStarImport, "UnusedImports", "RedundantImport" modules to the main checkstyle configuration (not the "nohttp" one which has a separate goal) and only keep the changes required to make the build pass?

@bclozel bclozel self-assigned this Feb 24, 2025
@bclozel bclozel added the status: waiting-for-feedback We need additional information before we can continue label Feb 24, 2025
@pankratz76
Copy link
Author

pankratz76 commented Feb 24, 2025

Yes, of course. You mean the buildSrc/config/checkstyle/checkstyle.xml, right?
I only found the imports when activating the rule in the last remaining src/nohttp/checkstyle.xml.

I don't understand this, as the other two files already have the rule active, so I assume the inheritance is not applied, or I don't get it at all.

check yourself on the recent two commits.

> Task :checkstyleNohttp FAILED

image

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Feb 24, 2025
@bclozel
Copy link
Member

bclozel commented Feb 24, 2025

Sorry I misread the diff in your initial attempt. Import rules were already applied in the main checkstyle configuration it seems. If those don't apply in the framework-docs module there might be a checkstyle suppression rule that does this. Beside finding why those aren't applied in framework-docs, I don't think any other change is required.

@bclozel
Copy link
Member

bclozel commented Feb 24, 2025

Found it, it's not a suppression rule.
The checkstyle configuration is not applied at all to framework-docs because the conventions plugin is not applied on "framework-*" modules on purpose.

We could manually apply this conventions plugin in the framework-docs module. We might see a lot of issues that don't really make sense from a code snippet perspective, which is what framework-docs is all about.

@bclozel
Copy link
Member

bclozel commented Feb 24, 2025

I've tried to apply plugin: 'org.springframework.build.conventions' and I'm seeing Kotlin warnings breaking the build already. Our setup requires Java and Kotlin classes with the same name right now. For example, we'd need to suppress warnings for deprecated APIs or remove them entirely from the docs.

Our samples need some flexibility with the code style and I don't think that spending time on enforcing stricter code style in reference docs code snippets is time well spend on our side. I'm closing this PR.

@bclozel bclozel closed this Feb 24, 2025
@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants