Skip to content

[RSPEC-S1144] Remove unused private methods - rewrite #5739

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.

Sign up for GitHub

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

Jump to bottom
Closed
Pankraz76 wants to merge 1 commit into from
Closed

[RSPEC-S1144] Remove unused private methods - rewrite #5739

Pankraz76 wants to merge 1 commit into from

Conversation

@Pankraz76
Copy link
Contributor

@Pankraz76 Pankraz76 commented May 14, 2025 •
edited
Loading

[RSPEC-S1144] Remove unused private methods - rewrite

enabler:

[WARNING] The recipe produced 116 warning(s). Please report this to the recipe author.
[WARNING] These recipes would make changes to pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/private_method_in_inner_class_interface2.java:
[WARNING]     org.openrewrite.staticanalysis.RemoveUnusedPrivateMethods
[WARNING] These recipes would make changes to pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/private_method_in_inner_class_interface1.java:
[WARNING]     org.openrewrite.staticanalysis.RemoveUnusedPrivateMethods
[WARNING] Patch file available:
[WARNING]     /Users/vincent.potucek/IdeaProjects/pmd/target/rewrite/rewrite.patch
[WARNING] Estimate time saved: 10m
[WARNING] Run 'mvn rewrite:run' to apply the recipes.
image

This was referenced May 14, 2025
Merged
Closed
@Pankraz76 Pankraz76 force-pushed the RemoveUnusedPrivateMethods-rewrite branch from 870ef1b to 082df68 Compare May 14, 2025 11:17
@Pankraz76 Pankraz76 changed the title Remove unused private methods rewrite [RSPEC-S1144] Remove unused private methods - rewrite May 14, 2025
@Pankraz76 Pankraz76 marked this pull request as ready for review May 14, 2025 11:17
@Pankraz76 Pankraz76 force-pushed the RemoveUnusedPrivateMethods-rewrite branch from 082df68 to 6aafe05 Compare May 14, 2025 11:21
Pankraz76
Pankraz76 commented May 14, 2025
View reviewed changes
pom.xml
<exclusion>pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/private_method_in_inner_class_interface1.java</exclusion>
<exclusion>pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/ast/jdkversiontests/private_method_in_inner_class_interface2.java</exclusion>
<exclusion>pmd-modelica/src/test/java/net/sourceforge/pmd/lang/modelica/resolver/ModelicaResolverTest.java</exclusion>
</exclusions>
Copy link
Contributor Author

@Pankraz76 Pankraz76 May 14, 2025

Choose a reason for hiding this comment

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

undo when:

@Pankraz76 Pankraz76 mentioned this pull request May 14, 2025
Closed
@Pankraz76
Copy link
Contributor Author

Pankraz76 commented May 14, 2025

Warning: These recipes would make changes to pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/cpd/testdata/SimpleClass.java:

@Pankraz76 Pankraz76 force-pushed the RemoveUnusedPrivateMethods-rewrite branch from 6aafe05 to c8be946 Compare May 14, 2025 19:29
@Pankraz76
Copy link
Contributor Author

Pankraz76 commented May 14, 2025

Warning: These recipes would make changes to pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/cpd/testdata/SimpleClass.java:

why this one found only in CI?

@github-actions
Copy link

github-actions bot commented May 14, 2025

Documentation Preview

No regression tested rules have been changed.

(comment created at 2025-05-14 19:51:35+00:00 for c8be946)

oowekyala
oowekyala requested changes May 15, 2025
View reviewed changes
Copy link
Member

@oowekyala oowekyala left a comment

Choose a reason for hiding this comment

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

I don't really want to add this plugin to the build. It is inconvenient to have so many exclusions, and I would rather the build system just warn me about mistakes.

We already have a PMD check in our configuration to find unused private methods so I don't think it is adding anything to add this other check. At most it would be interesting to investigate why our rule is not reporting the couple of examples you found in #5738, and improve the rule that way.

@Pankraz76
Copy link
Contributor Author

Pankraz76 commented May 15, 2025 •
edited
Loading

many exclusions

Most will be reworked and suppression is normal part of such tools, like in PMD as well, therefor this is not valid argument to me.

investigate why our rule

yes thats clearly why there is a need.

Unused method is only one of many best practises.

This PR is about giving attention and then see, which recipes would help.

@Pankraz76
Copy link
Contributor Author

Pankraz76 commented May 17, 2025

recent discovery seems fishy - the king is dead.

then checkstyle-stack using PMD/spotbugs seems only option - long live the king:

@Pankraz76 Pankraz76 closed this May 17, 2025
@Pankraz76 Pankraz76 mentioned this pull request Jul 13, 2025
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oowekyala oowekyala oowekyala requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Pankraz76 @oowekyala