Issue #14424: Add option to skip validation based on list of annotations #14701

@Lmh-java, please reply "done" to each review item, to give us clear singnal your arer ready for review and nothing is missed to be addressed. Do not resolve conversations, maintainers will do this as confirmation that is done as expected.

romani

last minors:

src/xdocs/checks/design/hideutilityclassconstructor.xml.template

...rces/com/puppycrawl/tools/checkstyle/checks/design/hideutilityclassconstructor/Example1.java

...ain/java/com/puppycrawl/tools/checkstyle/checks/design/HideUtilityClassConstructorCheck.java

romani · 2024-03-28T12:43:04Z

@Lmh-java , please reply "done" to each review item, it help a lot.

romani

last:

...ain/java/com/puppycrawl/tools/checkstyle/checks/design/HideUtilityClassConstructorCheck.java

romani · 2024-03-28T12:48:50Z

GitHub, generate website

github-actions · 2024-03-28T12:54:52Z

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

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/80b09ce_2024125225/checks/design/hideutilityclassconstructor.html#Examples

romani

items:

...rces/com/puppycrawl/tools/checkstyle/checks/design/hideutilityclassconstructor/Example1.java

Lmh-java · 2024-03-28T15:53:46Z

@Lmh-java , please reply "done" to each review item, it help a lot.

@romani Yes, I believe I have replied "done" to each review item after you suggested it from last time. I just want to confirm that I am doing that correctly.

romani · 2024-03-28T20:18:00Z

Please reply "done" as confirmation that get best practice of PR review

Lmh-java · 2024-03-28T20:23:25Z

Please reply "done" as confirmation that get best practice of PR review

~~That's wired, I have always been replying with "done" since you suggested. Can you see them?~~

Okay, I see. I need to click review changes to submit all comments. I thought pending means my comment is waiting to get reviewed by maintainers. I've always been replying, but forgot to submit the comments. Sorry for taking so long to figure it out.

rnveach

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

This only shows regression. We need a report for the new property as well. Configure it for some annotation. You can use the annotation specified in the issue.

...ain/java/com/puppycrawl/tools/checkstyle/checks/design/HideUtilityClassConstructorCheck.java

src/xdocs/checks/design/hideutilityclassconstructor.xml.template

Lmh-java · 2024-04-19T05:09:16Z

GitHub, generate report

github-actions · 2024-04-19T05:29:51Z

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

Lmh-java · 2024-04-19T05:33:42Z

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

This only shows regression. We need a report for the new property as well. Configure it for some annotation. You can use the annotation specified in the issue.

@rnveach
The regression report for the new property is here.

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/75e5ab1_2024052933/reports/diff/index.html
All changes are related to the new property added in this PR.

I have to use a different project to test this new feature. I have gone through our default regression project list, but didn't see any valid test case satisfying all the conditions here.

rnveach · 2024-04-24T16:54:27Z

gone through our default regression project list, but didn't see any valid test case

Report is fine, just that if original regression showed nothing for this change I would still leave the projects in there as proof they don't show anything.

Lmh-java · 2024-04-24T17:55:42Z

GitHub, generate report

Lmh-java · 2024-04-24T18:25:25Z

Github, generate site

github-actions · 2024-04-24T18:40:19Z

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

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/6bcff74_2024183836/checks/design/hideutilityclassconstructor.html#Examples

github-actions · 2024-04-24T18:44:58Z

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

Lmh-java · 2024-04-24T19:12:10Z

GitHub, generate report

github-actions · 2024-04-24T19:56:08Z

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

Lmh-java · 2024-04-26T06:08:36Z

@rnveach
The correct regression report is here. https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/6bcff74_2024195534/reports/diff/index.html.

There are no existing projects in the default list with this test case. They are either not having any annotation, or they are already final class, or they contain some non-static method, etc. The only changes is in the one project I configured additionally. Some main classes with SpringBootApplication annotation are ignored.

I think I messed up with the previous regression report. I have had one additional project of local-checkstyle in the project list by mistake.

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

I noticed that we are not using local-checkstyle|local| config in the default project list. And if I use that, there will be some random differences.
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/75e5ab1_2024184427/reports/diff/local-checkstyle/index.html.
If I run locally to compare between these two configs, there are no those difference as shown in the report. Do you have any thoughts on why this happens?

rnveach · 2024-04-29T18:32:04Z

I noticed that we are not using local-checkstyle|local| config in the default project list. And if I use that, there will be some random differences.
If I run locally to compare between these two configs, there are no those difference as shown in the report.
local-checkstyle|local|../../../|master

It depends on where this local is coming from and if anything else is using it. If this is the same git repository provided to regression as the basis for checkstyle, then you have to know that part of the work regression does to do its magic is to switch branches in that git repository. It has to switch between master and the PR to install both versions of checkstyle in order to run them. That switching may cause issues with you defining it as a property to test on as it is only expecting to stay in master and not move around.

This is an issue to be able to set up a way to do regression on the changes in the PR itself.

rnveach

Last things on documentation.

rnveach · 2024-04-29T18:35:55Z

...rces/com/puppycrawl/tools/checkstyle/checks/design/hideutilityclassconstructor/Example2.java

+  <module name="TreeWalker">
+    <module name="HideUtilityClassConstructor">
+      <property name="ignoreAnnotatedBy"
+        value="SpringBootApplication, java.lang.Deprecated" />


Annotation names provided in this property must exactly match the annotation names on the classes. If the target class has annotations specified with their fully qualified names (including package), the annotations in this property should also be specified with their fully qualified names. Similarly, if the target class has annotations specified with their simple names, this property should contain the annotations with the same simple names.

Show some of these issues in your example.

I assume Deprecated by itself won't work. Show that and put a comment to that effect.

Can you also provide me where we said we would be so strict?

I assume Deprecated by itself won't work.

Only Deprecated will not work if it doesn't match the annotation used.

Can you also provide me where we said we would be so strict?

We mentioned this at #14424 (comment). We agreed to do exact match first as #14553. We might do pattern matching later, but they need to solve the problem of #5434 first.

yes, I am fine to be strict for first step, we can improve later if sombody start to complain.
We just need to document this.

rnveach · 2024-04-29T18:36:32Z

src/xdocs/checks/design/hideutilityclassconstructor.xml

+              <td>Ignore classes annotated with the specified annotation(s). Annotation names provided in this property must exactly match the annotation names on the classes. If the target class has annotations specified with their fully qualified names (including package), the annotations in this property should also be specified with their fully qualified names. Similarly, if the target class has annotations specified with their simple names, this property should contain the annotations with the same simple names.</td>
+              <td><a href="../../property_types.html#String.5B.5D">String[]</a></td>
+              <td><code>{}</code></td>
+              <td>10.16.0</td>


Version needs updated.

should be 10.17.0

romani · 2024-05-11T18:09:34Z

@Lmh-java , please help us to finish this PR, we are so close to merge state.
looks like only version should be changed.

…f annotations

Lmh-java · 2024-05-12T16:17:40Z

@Lmh-java , please help us to finish this PR, we are so close to merge state.
looks like only version should be changed.

Sorry, I was quite occupied with other stuffs.

I have updated a new example for #14701 (comment) and updated the version.

Lmh-java · 2024-05-12T16:18:28Z

GitHub, generate site

Lmh-java force-pushed the minghao/skip-validation-with-hide-util-class branch from 8c907a3 to f763b2e Compare March 22, 2024 04:01

Lmh-java marked this pull request as ready for review March 22, 2024 17:47

nrmancuso self-assigned this Mar 26, 2024

romani requested changes Mar 26, 2024

View reviewed changes

...ain/java/com/puppycrawl/tools/checkstyle/checks/design/HideUtilityClassConstructorCheck.java Outdated Show resolved Hide resolved

romani requested changes Mar 26, 2024

View reviewed changes

nrmancuso assigned romani and unassigned nrmancuso Mar 26, 2024

Lmh-java force-pushed the minghao/skip-validation-with-hide-util-class branch from f763b2e to dfc82c6 Compare March 26, 2024 18:27

Lmh-java requested a review from romani March 26, 2024 18:42

romani requested changes Mar 27, 2024

View reviewed changes

Lmh-java force-pushed the minghao/skip-validation-with-hide-util-class branch from dfc82c6 to 80b09ce Compare March 28, 2024 04:46

Lmh-java requested a review from romani March 28, 2024 06:15

romani requested changes Mar 28, 2024

View reviewed changes

...ain/java/com/puppycrawl/tools/checkstyle/checks/design/HideUtilityClassConstructorCheck.java Outdated Show resolved Hide resolved

romani requested changes Mar 28, 2024

View reviewed changes

...rces/com/puppycrawl/tools/checkstyle/checks/design/hideutilityclassconstructor/Example1.java Outdated Show resolved Hide resolved

Lmh-java force-pushed the minghao/skip-validation-with-hide-util-class branch from 80b09ce to c52a701 Compare March 28, 2024 16:20

Lmh-java requested a review from romani March 28, 2024 18:04

romani assigned rnveach and unassigned nrmancuso Apr 17, 2024

rnveach requested changes Apr 18, 2024

View reviewed changes

...ain/java/com/puppycrawl/tools/checkstyle/checks/design/HideUtilityClassConstructorCheck.java Outdated Show resolved Hide resolved

src/xdocs/checks/design/hideutilityclassconstructor.xml.template Outdated Show resolved Hide resolved

Lmh-java force-pushed the minghao/skip-validation-with-hide-util-class branch 3 times, most recently from 122dfa9 to 75e5ab1 Compare April 18, 2024 23:45

Lmh-java requested a review from rnveach April 19, 2024 05:33

Lmh-java force-pushed the minghao/skip-validation-with-hide-util-class branch from 75e5ab1 to 6bcff74 Compare April 24, 2024 18:22

rnveach requested changes Apr 29, 2024

View reviewed changes

rnveach added the awaiting changes label May 5, 2024

romani closed this May 11, 2024

romani reopened this May 11, 2024

Lmh-java force-pushed the minghao/skip-validation-with-hide-util-class branch from 6bcff74 to 4218a94 Compare May 12, 2024 16:09

Issue checkstyle#14424: Add option to skip validation based on list o…

70bd1b7

…f annotations

Lmh-java force-pushed the minghao/skip-validation-with-hide-util-class branch from 4218a94 to 70bd1b7 Compare May 12, 2024 16:13

Issue #14424: Add option to skip validation based on list of annotations #14701

Are you sure you want to change the base?

Issue #14424: Add option to skip validation based on list of annotations #14701

Conversation

Lmh-java commented Mar 22, 2024 • edited

Lmh-java commented Mar 22, 2024

github-actions bot commented Mar 22, 2024

Lmh-java commented Mar 22, 2024

github-actions bot commented Mar 22, 2024

Lmh-java commented Mar 22, 2024

nrmancuso commented Mar 26, 2024

github-actions bot commented Mar 26, 2024

romani left a comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

romani commented Mar 27, 2024

romani left a comment

Choose a reason for hiding this comment

romani commented Mar 28, 2024

romani left a comment

Choose a reason for hiding this comment

romani commented Mar 28, 2024

github-actions bot commented Mar 28, 2024

romani left a comment

Choose a reason for hiding this comment

Lmh-java commented Mar 28, 2024 • edited

romani commented Mar 28, 2024

Lmh-java commented Mar 28, 2024 • edited

rnveach left a comment

Choose a reason for hiding this comment

Lmh-java commented Apr 19, 2024

github-actions bot commented Apr 19, 2024

Lmh-java commented Apr 19, 2024 • edited

rnveach commented Apr 24, 2024

Lmh-java commented Apr 24, 2024

Lmh-java commented Apr 24, 2024

github-actions bot commented Apr 24, 2024

github-actions bot commented Apr 24, 2024

Lmh-java commented Apr 24, 2024

github-actions bot commented Apr 24, 2024

Lmh-java commented Apr 26, 2024

rnveach commented Apr 29, 2024

rnveach left a comment

Choose a reason for hiding this comment

rnveach Apr 29, 2024

Choose a reason for hiding this comment

rnveach Apr 29, 2024

Choose a reason for hiding this comment

Lmh-java May 3, 2024

Choose a reason for hiding this comment

Lmh-java May 3, 2024

Choose a reason for hiding this comment

romani May 3, 2024 • edited

Choose a reason for hiding this comment

rnveach Apr 29, 2024

Choose a reason for hiding this comment

romani May 3, 2024

Choose a reason for hiding this comment

romani commented May 11, 2024 • edited

Lmh-java commented May 12, 2024

Lmh-java commented May 12, 2024

Lmh-java commented Mar 22, 2024 •

edited

Lmh-java commented Mar 28, 2024 •

edited

Lmh-java commented Mar 28, 2024 •

edited

Lmh-java commented Apr 19, 2024 •

edited

romani May 3, 2024 •

edited

romani commented May 11, 2024 •

edited