Issue #14981: Add new property to skip FinalLocalVariable violations … by mahfouz72 · Pull Request #14983 · checkstyle/checkstyle

mahfouz72 · 2024-06-13T14:25:44Z

added a new property named validateUnnamedVariables to control whether to validate unnamed variables the default value false

Diff Regression config: https://gist.githubusercontent.com/mahfouz72/afd57aa6e51ca161b7927acd959e5211/raw/2cceba105232745543e237d50c0c4a38883be735/check.xml

Diff Regression patch config: https://gist.githubusercontent.com/mahfouz72/33967bf86e4a29cafa429771b0e95808/raw/a13fee304bdf4e853b3de5dd71a3c4aece177445/check_patch.xml

Diff Regression projects: https://gist.githubusercontent.com/mahfouz72/a3d0af030c8f5efd0d8a39f2c14750bc/raw/a3424ad9b6f722e5de1a927e3d85e383101b2b93/projects-to-test-on.properties

mahfouz72 · 2024-06-13T14:26:04Z

Github, generate site

mahfouz72 · 2024-06-13T14:43:44Z

Github, generate report

github-actions · 2024-06-13T14:49:15Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/54869e0_2024143859/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/54869e0_2024143859/checks/coding/finallocalvariable.html#Properties

mahfouz72 · 2024-06-13T15:01:21Z

Github, generate site

github-actions · 2024-06-13T15:01:26Z

Report generation failed on phase "make_report",
step "Generate report".
Link: https://github.com/checkstyle/checkstyle/actions/runs/9501765801

mahfouz72 · 2024-06-13T15:01:31Z

Github, generate report

github-actions · 2024-06-13T15:21:18Z

Report generation failed on phase "make_report",
step "Generate report".
Link: https://github.com/checkstyle/checkstyle/actions/runs/9502032213

github-actions · 2024-06-13T15:21:24Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/5237ebb_2024152016/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/5237ebb_2024152016/checks/coding/finallocalvariable.html#Properties

mahfouz72 · 2024-06-13T15:45:20Z

@rnveach @nrmancuso pitest-utils is red because FinalLocalVariableTest is not in in targetTests of utils profile. I can add it and I can move isUnamed() to LocalVariableCheck itself and I can make a junit test in TokenUtilTest but @romani said that pure junit should be avoided

I would go for adding this test class to the targetTest. I think all check tests should be there
also, please help why report generation is failing configuration looks good to me

rnveach · 2024-06-13T15:57:17Z

FinalLocalVariableTest is not in in targetTests of utils profile
I would go for adding this test class to the targetTest. I think all check tests should be there
also, please help why report generation is failing configuration looks good to me

I am not seeing any updates to the POM in this PR, so I am not sure how you say configuration is good. It sounds like POM should add FinalLocalVariableTest to the pitest-utils part of the POM.

nrmancuso

I am good to merge this after one item, and successful diff report:

nrmancuso · 2024-06-13T16:00:24Z

+    public static boolean isUnnamed(DetailAST identifierAst) {
+        return "_".equals(identifierAst.getText());
+    }


Let's wait for utility until we need this in more than one place

This condition is no simple, I am not even sure it is worthy of having a utility method

ok, yes it is very simple I just put it there because we will need to use it a lot instead of putting it in the checks classes

@nrmancuso done

mahfouz72 · 2024-06-13T16:43:08Z

I am not seeing any updates to the POM in this PR, so I am not sure how you say configuration is good. It sounds like POM should add FinalLocalVariableTest to the pitest-utils part of the POM.

@rnveach here I was talking about the report generation and the regression config

Error: Failed to execute goal org.apache.maven.plugins:maven-site-plugin:3.3:site (default-site) on project sample: Error during page generation: Error rendering Maven report: Failed during checkstyle execution: Failed during checkstyle configuration: unable to parse configuration stream - The markup in the document following the root element must be well-formed.:45:6 -> [Help 1]

mahfouz72 · 2024-06-13T16:48:07Z

+    /**
+     * Control whether to check
+     * <a href="https://docs.oracle.com/javase/specs/jls/se21/preview/specs/unnamed-jls.html">
+     * unnamed variables </a>.
+     */


JLS doesn't have a separate section for this I just went to (https://docs.oracle.com/javase/specs/index.html) and copied the link of the last review for this feature is this ok?

Is there some way we can use a non-preview JLS? Nothing we are doing is preview only, right?

Is there some way we can use a non-preview JLS?

I don't think so. All items listed here are previews. we can use a non-perview from here if the feature has a separate new section

rnveach · 2024-06-13T16:49:38Z

https://gist.githubusercontent.com/mahfouz72/afd57aa6e51ca161b7927acd959e5211/raw/545956596861401a2bb555f7c7a08b8421247f2c/check.xml
Line 26. There is an extra </module>.

mahfouz72 · 2024-06-13T16:57:05Z

ohh, Thank you

nrmancuso

Noticed a few more:

nrmancuso · 2024-06-13T16:57:21Z

+        }
+        final int _ = sideEffect();
+        int _ = sideEffect();
+        int _result = sideEffect(); // violation


Please add violation messages, always

nrmancuso · 2024-06-13T16:58:01Z

+            <tr>
+              <td>validateUnnamedVariables</td>
+              <td>Control whether to check <a href="https://docs.oracle.com/javase/specs/jls/se21/preview/specs/unnamed-jls.html">
+ unnamed variables </a>.</td>


Why do we always have a space between s and </a>?

nothing, removed

@nrmancuso I noticed that the spaces come again when I made mvn verify I don't why is it something related to the xml files generation or something?

Remove it from the setter in the javadoc.

is it generated from the setter? I thought from the Javadoc on class field

The javadoc on the class is derived from the others as it is an accumulation of multiple pieces, mostly from the xdoc.

mahfouz72 · 2024-06-13T18:06:09Z

Github, generate report

mahfouz72 · 2024-06-13T18:06:17Z

Github, generate site

github-actions · 2024-06-13T18:47:56Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/0788aad_2024184647/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/0788aad_2024184647/checks/coding/finallocalvariable.html#Properties

github-actions · 2024-06-13T19:41:32Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/0788aad_2024194101/reports/diff/index.html

mahfouz72 · 2024-06-13T22:22:44Z

Github, generate site

github-actions · 2024-06-13T22:39:27Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/9e0fd51_2024223718/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/9e0fd51_2024223718/checks/coding/finallocalvariable.html#Properties

…iolations on unnamed variables

mahfouz72 · 2024-06-15T11:24:38Z

Github, generate site

github-actions · 2024-06-15T11:33:57Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/bb18ca9_2024113245/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/bb18ca9_2024113245/checks/coding/finallocalvariable.html#Properties

mahfouz72 force-pushed the finallocalvariable branch from d5ab5a4 to 54869e0 Compare June 13, 2024 14:37

mahfouz72 force-pushed the finallocalvariable branch from 54869e0 to aff88c0 Compare June 13, 2024 14:48

mahfouz72 force-pushed the finallocalvariable branch from aff88c0 to 5237ebb Compare June 13, 2024 15:00

rnveach assigned nrmancuso Jun 13, 2024

nrmancuso suggested changes Jun 13, 2024

View reviewed changes

mahfouz72 force-pushed the finallocalvariable branch from 5237ebb to 5689f7d Compare June 13, 2024 16:45

mahfouz72 commented Jun 13, 2024

View reviewed changes

mahfouz72 force-pushed the finallocalvariable branch from 5689f7d to 222b855 Compare June 13, 2024 16:49

nrmancuso suggested changes Jun 13, 2024

View reviewed changes

mahfouz72 force-pushed the finallocalvariable branch 2 times, most recently from 3502ae0 to 0788aad Compare June 13, 2024 17:55

mahfouz72 force-pushed the finallocalvariable branch from 0788aad to 9e0fd51 Compare June 13, 2024 22:18

nrmancuso approved these changes Jun 14, 2024

View reviewed changes

nrmancuso assigned rnveach and unassigned nrmancuso Jun 14, 2024

rnveach reviewed Jun 14, 2024

View reviewed changes

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheck.java Outdated

Issue checkstyle#14981: Add new property to skip FinalLocalVariable v…

bb18ca9

…iolations on unnamed variables

mahfouz72 force-pushed the finallocalvariable branch from 9e0fd51 to bb18ca9 Compare June 15, 2024 11:14

mahfouz72 mentioned this pull request Jun 15, 2024

Standardize @Since Version Formatting to Three-Digit Versions #14995

Closed

rnveach approved these changes Jun 18, 2024

View reviewed changes

rnveach merged commit 104cf77 into checkstyle:master Jun 18, 2024

mahfouz72 deleted the finallocalvariable branch July 4, 2024 00:33

Uh oh!

Conversation

mahfouz72 commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mahfouz72 commented Jun 13, 2024

Uh oh!

mahfouz72 commented Jun 13, 2024

Uh oh!

github-actions Bot commented Jun 13, 2024

Uh oh!

mahfouz72 commented Jun 13, 2024

Uh oh!

github-actions Bot commented Jun 13, 2024

Uh oh!

mahfouz72 commented Jun 13, 2024

Uh oh!

github-actions Bot commented Jun 13, 2024

Uh oh!

github-actions Bot commented Jun 13, 2024

Uh oh!

mahfouz72 commented Jun 13, 2024

Uh oh!

rnveach commented Jun 13, 2024

Uh oh!

nrmancuso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahfouz72 commented Jun 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnveach commented Jun 13, 2024

Uh oh!

mahfouz72 commented Jun 13, 2024

Uh oh!

nrmancuso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahfouz72 commented Jun 13, 2024

Uh oh!

mahfouz72 commented Jun 13, 2024

Uh oh!

github-actions Bot commented Jun 13, 2024

Uh oh!

github-actions Bot commented Jun 13, 2024

Uh oh!

mahfouz72 commented Jun 13, 2024

Uh oh!

github-actions Bot commented Jun 13, 2024

Uh oh!

Uh oh!

mahfouz72 commented Jun 13, 2024 •

edited

Loading