Fix UnusedMethod & Finally#34881
Conversation
ov7a
left a comment
ov7a
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
Please, split the PR.
Removing unused private methods should be fine. A PR with these changes will be accepted if it pass the tests.
Introducing PMD into our build is a different story. First, it should be done as a part of sanity check. Second, CheckStyle has UnusedPrivateMethodCheck. So instead of introducing a new tool, please adjust already existing ones.
ov7a
added
the
pending:feedback
This comment was marked as resolved.
This comment was marked as resolved.
github-actions
Bot
removed
the
pending:feedback
|
testing ci run that should fail due to unused method
Working as expected to PMD just needs to be adjusted. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
kindly request feedback. Thanks for consideration. happy ci run (including Rewrite as comparison): |
I don't think this is the right place to discuss Groovy support in PMD. But at the moment Groovy is only supported in CPD. And since I'm already here: you're right that CheckStyle may fail to detect missing unused methods, but Error Prone that's also configured in this build should catch those with https://errorprone.info/bugpattern/UnusedMethod . This rule is currently disabled in some modules, e.g. https://github.com/gradle/gradle/blob/master/subprojects/core/build.gradle.kts#L59 . You should be able to enable it and remove unused methods if that's something Gradle maintainers are interested in. |
ov7a
left a comment
ov7a
left a comment
There was a problem hiding this comment.
❌ Let's keep the discussion focused.
I agree with @zbynek that it's not the right place to discuss Groovy support in PMD.
I think what you mean is that it could only be done there.
... That said, this really makes the most sense when combined with some kind of quality gate
I'm not sure if I understand what you mean by that. We have a quality gate already.
testing ci run that should fail due to unused method unusedTestCi:
Ok, I assume that can be used as some proof that PMD is better with that check compared to CheckStyle. I'm still not convinced that we should introduce PMD just because of that check, though.
You should be able to enable it and remove unused methods if that's something Gradle maintainers are interested in.
@zbynek Thank you for your suggestion and constructive feedback!
Indeed, errorprone is enabled in our build, and working on cleaning up disabled checks is something that we plan to do eventually.
@Pankraz76 given that, we don't have a good reason to introduce PMD. You're welcome to remove errorprone exceptions and make relevant code changes.
Pankraz76
left a comment
Pankraz76
left a comment
There was a problem hiding this comment.
concerning/disturbing api change.
|
thanks for rebase. |
|
@bot-gradle test this |
This comment has been minimized.
This comment has been minimized.
|
@Pankraz76 Thanks for working on this. If the tests pass, I'll merge this. FTR, this PR only addresses issues in the |
|
The following builds have failed: |
|
@Pankraz76 Please take a look at failed tests |
Signed-off-by: Vincent Potucek <[email protected]>
5 participants
Fix
UnusedMethod&FinallyContributor Checklist
<subproject>/src/integTest) to verify changes from a user perspective.<subproject>/src/test) to verify logic../gradlew sanityCheck../gradlew <changed-subproject>:quickTest.Reviewing cheatsheet
Before merging the PR, comments starting with
ref:
Rewrite & PMDcovering S1144: Unused "private" methods should be removed opensearch-project/OpenSearch#18791Rewritecovering S1144: Unused "private" methods should be removed checkstyle/checkstyle#17545Rewrite:UpgradeToJava17checkstyle/checkstyle#17730Spotlessto reduce the pain on fixing checkstyle issues checkstyle/checkstyle#17733Spotlessto reduce the pain on fixing checkstyle issues checkstyle/checkstyle#17732Spotlessto reduce the pain on fixing checkstyle issues #34880@zbynek can you please tell if PMD should find these groovy issues we well? Assuming we check for bytecode and docs tell it should be possible to detect these.