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

remove UnusedLocalVariable #34489

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

Conversation

pankratz76
Copy link

@pankratz76 pankratz76 commented Feb 25, 2025

Encounter unused code with help of CS module UnusedLocalVariable.

Wondering why the CS config needs to be duplicated. The rules seem not to be applied, some of the findings only emerge after adding the corresponding module again.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 25, 2025
Copy link
Author

@pankratz76 pankratz76 left a comment

Choose a reason for hiding this comment

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

question

@@ -1,10 +1,10 @@
root = true

[*.{adoc,bat,groovy,html,java,js,jsp,kt,kts,md,properties,py,rb,sh,sql,svg,txt,xml,xsd}]
[*]
charset = utf-8
Copy link
Author

Choose a reason for hiding this comment

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

Just for clarification, is the intent to not make this the default?

I have never seen it otherwise, only globally, with the most common encoding being UTF-8.

This is new to me—my previous knowledge was that properties files were the only files not normally encoded in UTF-8, but now even they have switched.

"As of Java 9, properties files switched from being loaded using ISO-8859-1."
Source

@pankratz76 pankratz76 changed the title rewise UnusedLocalVariable apply UnusedLocalVariable Feb 25, 2025
@pankratz76 pankratz76 force-pushed the UnusedLocalVariable-apply_RemoveUnusedPrivateFields branch from 520a35a to 6a96950 Compare March 5, 2025 07:43
String value = "Sample";
Method[] methods = String.class.getMethods();
methods = Integer.class.getMethods();
String.class.getMethods();
Copy link
Author

Choose a reason for hiding this comment

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

not sure if the test is still working as expected, as I could not execute it locally.

@pankratz76 pankratz76 force-pushed the UnusedLocalVariable-apply_RemoveUnusedPrivateFields branch from 6a96950 to eb3f8cc Compare March 5, 2025 08:29
@pankratz76 pankratz76 marked this pull request as ready for review March 5, 2025 08:30
.editorconfig Outdated
charset = utf-8
end_of_line = lf
indent_style = space
Copy link
Author

Choose a reason for hiding this comment

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

is the intend space for all others as it was only specified on an explicit set?

Suggested change
indent_style = space
indent_style = tab


<!-- Imports -->
<module name="AvoidStarImport"/>
<module name="UnusedImports"/> <!-- review: needs to be explicit activated -->
Copy link
Author

Choose a reason for hiding this comment

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

needs to be explicit activated, so the root configuration does not seem to work.

@@ -7145,8 +7145,7 @@ public TestClass9(int i) {
static class HttpServlet3RequestFactory {

static Servlet3SecurityContextHolderAwareRequestWrapper getOne() {
HttpServlet3RequestFactory outer = new HttpServlet3RequestFactory();
return outer.new Servlet3SecurityContextHolderAwareRequestWrapper();
Copy link
Author

Choose a reason for hiding this comment

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

possible false positive on UnusedLocalVariable checkstyle/checkstyle#16419

@pankratz76 pankratz76 force-pushed the UnusedLocalVariable-apply_RemoveUnusedPrivateFields branch 2 times, most recently from 4c99d76 to c0b35a2 Compare March 9, 2025 14:01
Signed-off-by: Vincent Potucek <[email protected]>
@pankratz76 pankratz76 force-pushed the UnusedLocalVariable-apply_RemoveUnusedPrivateFields branch from c0b35a2 to 13bcd56 Compare March 9, 2025 14:02
@pankratz76
Copy link
Author

even tho its only housekeeping and therefor low prio, is this any good? @bclozel

this PR is kind of the follow up of #34477 to make a clear statement in the config to explicitly determine a different rule set.

@SuppressWarnings("unused")
String result2 = this.restTemplate.getForObject("/number", String.class);
// result == "2"
assertThat(this.restTemplate.getForObject("/number", String.class)).isEqualTo("1");
Copy link
Author

Choose a reason for hiding this comment

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

this is some real benefit, imho. Testing the real assertion rather commenting about it

@pankratz76 pankratz76 changed the title apply UnusedLocalVariable remove UnusedLocalVariable Apr 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged or decided on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants