Issue #11338: FinalClassCheck should report private classes without c… #12737

Kevin222004 · 2023-02-11T20:16:24Z

Issue #11338: FinalClassCheck should report private classes without constructor

Diff Regression config: https://gist.githubusercontent.com/Kevin222004/2ce4726e677546482c436a000f4b01d9/raw/3faec59c1d2757c5398a5b325d32e43f12c9ca73/finalclass.xml

Kevin222004 · 2023-02-11T20:26:05Z

GitHub, generate report

github-actions · 2023-02-11T21:35:36Z

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

romani · 2023-02-12T15:29:53Z

@Kevin222004 , please send PR to our plugin to fix https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/69b1d89_2023212857/reports/diff/checkstyle-sonar/index.html
and to our sevntu project to fix not-Inputs https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/69b1d89_2023212857/reports/diff/sevntu-checkstyle/index.html

this bug fix is very aggressive :), as nobody noticed this or care much on this "final" in private classes.

romani

if amount of violation is such massive, can we offer user some Xpath suppression to let them not be blocked to fix all code to upgrade to new version of checkstyle ?

items:

src/main/java/com/puppycrawl/tools/checkstyle/checks/design/FinalClassCheck.java

...ces/com/puppycrawl/tools/checkstyle/checks/design/finalclass/InputFinalClassPrivateCtor.java

Kevin222004 · 2023-02-14T10:35:51Z

@romani all the pr in other repo has been merged. is their any way of ci get passed of seventu.checkstyle repo to not block this pr

Kevin222004 · 2023-02-14T16:44:42Z

@romani please look at the conversation jqno/equalsverifier#770 (comment)

rnveach · 2023-02-14T19:02:39Z

is their any way of ci get passed of seventu.checkstyle repo to not block this pr

Edit:
Sevntu's CI is not a blocker.

romani · 2023-02-16T14:32:36Z

please move all updates for "final" to separate PR and name commit as "supplemental: ...." we will merge it sooner
example 50e41f4

it will reduce size of this PR and ease review.

romani · 2023-02-17T05:10:33Z

@rnveach , please help to review this PR.

Kevin222004 · 2023-02-17T13:57:48Z

if amount of violation is such massive, can we offer user some Xpath suppression to let them not be blocked to fix all code to upgrade to new version of checkstyle ?

I am really sorry I missed this. I have to study this

romani · 2023-05-25T13:02:44Z

We are waiting for state when such conversation is resolved.

nrmancuso · 2023-05-27T11:21:01Z

We are waiting for state when such conversation is resolved.

@romani to be clear, this means that we are waiting on resolution of #12737 (comment) to continue review?

romani · 2023-05-27T22:38:15Z

@nrmancuso , please do review, looks like this item is not very critical, just clarification.

rnveach · 2023-05-28T01:33:51Z

Item seeking clarification on probably either means documentation is not clear enough, or we may need a new issue.

nrmancuso

Items:

src/main/java/com/puppycrawl/tools/checkstyle/checks/design/FinalClassCheck.java

nrmancuso · 2023-05-28T12:59:11Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/design/FinalClassCheck.java

+ *       Class is declared as {@code abstract}.
+ *   </li>
+ *   <li>
+ *       Class is Super class of some Anonymous inner class.
+ *   </li>


These also seem like obvious reasons to not place violations on a class; the compiler enforces this.

Really, it seems like this whole section is unnecessary since it can be summed up as "we don't place violations on code that already meets criteria (class is final) or is not compilable", since both conditions are already implied by (a) all checks work like this, and (b) Checkstyle should not be ran on code that doesn't compile (this is already documented elsewhere).

we don't place violations on code that already meets criteria (class is final) or is not compilable

you are right this are all the case as you mentioned above.

@nrmancuso should we remove the entire section ?

Probably, @rnveach any thoughts on this?

I was the one who requested this list.

Some things weren't clear and once I asked more about one of the items, I think its like a bug. #12737 (comment)
I don't see Class is Super class of some Anonymous inner class being clear to me.

The first 2 just make sense since we started a list, but I am ok with them removed.

@Kevin222004 ping

I have removed the final and abstract one.

I have chnaged the sentence as Class is extended by another class in the same file

#12737 (comment)

@rnveach again the purpose of

I don't see Class is Super class of some Anonymous inner class being clear to me. #12737 (comment)

is to show #12737 (comment)

One mistake that I have made is, have blindly copy paste some names without thinking in docs which is misleading in code as well #12737 (comment)

@nrmancuso @rnveach ping lets complete this

@rnveach ping please continue

src/main/java/com/puppycrawl/tools/checkstyle/checks/design/FinalClassCheck.java

nrmancuso

Final item from me:

src/main/java/com/puppycrawl/tools/checkstyle/checks/design/FinalClassCheck.java

Kevin222004 · 2023-06-17T16:42:27Z

https://app.circleci.com/pipelines/github/checkstyle/checkstyle/18998/workflows/1895fe46-9610-451b-86dc-4c89a91bf1d3/jobs/311465 Failure is not related to changes

nrmancuso · 2023-06-17T16:45:12Z

@rnveach please confirm you are good at #12737 (comment), I am good to merge

rnveach

Ensure we are rebased on the latest master and once this is fixed then I think we should be good.

rnveach · 2023-06-27T01:14:37Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/design/FinalClassCheck.java

+ *       Class is Super class of some Anonymous inner class.
+ *   </li>
+ *   <li>
+ *       Class is extended by another class in the same file..


Double period.

Kevin222004 · 2023-06-27T04:13:42Z

Failure is not related to changes

Kevin222004 · 2023-06-27T04:15:21Z

Github, generate site

github-actions · 2023-06-27T04:20:29Z

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

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/70b9b51_2023041848/checks.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/70b9b51_2023041848/checks/design/finalclass.html#Description

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/70b9b51_2023041848/checks/design/index.html#

… without constructor

Kevin222004 · 2023-06-27T17:48:34Z

@rnveach can you Please re-run semaphore all the ci are good

romani · 2023-06-28T03:49:05Z

All CIs are green. PR is ready for final review, @rnveach .

Kevin222004 force-pushed the FinalClass branch 3 times, most recently from e64eed0 to 36ce8fb Compare February 11, 2023 21:23

romani requested changes Feb 12, 2023

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/checks/design/FinalClassCheck.java Outdated Show resolved Hide resolved

...ces/com/puppycrawl/tools/checkstyle/checks/design/finalclass/InputFinalClassPrivateCtor.java Outdated Show resolved Hide resolved

Kevin222004 force-pushed the FinalClass branch from 36ce8fb to b97a432 Compare February 13, 2023 21:22

Kevin222004 force-pushed the FinalClass branch from b97a432 to 8a85122 Compare February 14, 2023 19:10

Kevin222004 mentioned this pull request Feb 15, 2023

minor: make private class final with default constructor-2 xwiki/xwiki-commons#380

Merged

Kevin222004 force-pushed the FinalClass branch from 8a85122 to 305fddd Compare February 15, 2023 07:18

Kevin222004 mentioned this pull request Feb 16, 2023

minor: make private class final with default constructor xwiki/xwiki-platform#2090

Merged

romani requested a review from rnveach February 17, 2023 05:10

romani assigned rnveach Feb 17, 2023

This was referenced Feb 17, 2023

minor: make private class final with default constructor xwiki/xwiki-rendering#251

Merged

supplemental: make private class final with default constructor #12750

Merged

Kevin222004 force-pushed the FinalClass branch from 305fddd to 676ddbe Compare February 18, 2023 19:50

Kevin222004 closed this Feb 18, 2023

Kevin222004 reopened this Feb 18, 2023

nrmancuso requested changes May 28, 2023

View reviewed changes

Kevin222004 force-pushed the FinalClass branch from f9f132c to b81b84b Compare June 14, 2023 04:30

nrmancuso requested changes Jun 17, 2023

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/checks/design/FinalClassCheck.java Outdated Show resolved Hide resolved

Kevin222004 force-pushed the FinalClass branch 2 times, most recently from 3995712 to 52b6892 Compare June 17, 2023 16:36

nrmancuso requested a review from rnveach June 17, 2023 16:44

nrmancuso assigned rnveach and unassigned nrmancuso Jun 17, 2023

nrmancuso approved these changes Jun 17, 2023

View reviewed changes

Kevin222004 force-pushed the FinalClass branch 3 times, most recently from fbb9cb5 to 26842e0 Compare June 23, 2023 15:47

rnveach requested changes Jun 27, 2023

View reviewed changes

Kevin222004 force-pushed the FinalClass branch from 26842e0 to 70b9b51 Compare June 27, 2023 04:02

Kevin222004 force-pushed the FinalClass branch from 70b9b51 to 49eb4ad Compare June 27, 2023 13:42

Issue checkstyle#11338: FinalClassCheck should report private classes…

e286af7

… without constructor

Kevin222004 force-pushed the FinalClass branch from 49eb4ad to e286af7 Compare June 27, 2023 16:38

rnveach approved these changes Jun 28, 2023

View reviewed changes

rnveach merged commit c3c7ab8 into checkstyle:master Jun 28, 2023
110 checks passed

Issue #11338: FinalClassCheck should report private classes without c… #12737

Issue #11338: FinalClassCheck should report private classes without c… #12737

Conversation

Kevin222004 commented Feb 11, 2023 • edited

Kevin222004 commented Feb 11, 2023

github-actions bot commented Feb 11, 2023

romani commented Feb 12, 2023 • edited

romani left a comment • edited

Choose a reason for hiding this comment

Kevin222004 commented Feb 14, 2023

Kevin222004 commented Feb 14, 2023

rnveach commented Feb 14, 2023 • edited

romani commented Feb 16, 2023 • edited

romani commented Feb 17, 2023

Kevin222004 commented Feb 17, 2023

romani commented May 25, 2023

nrmancuso commented May 27, 2023

romani commented May 27, 2023

rnveach commented May 28, 2023

nrmancuso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kevin222004 May 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kevin222004 Jun 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrmancuso left a comment

Choose a reason for hiding this comment

Kevin222004 commented Jun 17, 2023

nrmancuso commented Jun 17, 2023

rnveach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kevin222004 commented Jun 27, 2023

Kevin222004 commented Jun 27, 2023

github-actions bot commented Jun 27, 2023

Kevin222004 commented Jun 27, 2023

romani commented Jun 28, 2023

Kevin222004 commented Feb 11, 2023 •

edited

romani commented Feb 12, 2023 •

edited

romani left a comment •

edited

rnveach commented Feb 14, 2023 •

edited

romani commented Feb 16, 2023 •

edited

Kevin222004 May 28, 2023 •

edited

Kevin222004 Jun 14, 2023 •

edited