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

minor: make private class final with default constructor #2740

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

Kevin222004
Copy link
Contributor

minor: make private class final with default constructor

This is part of checkstyle/checkstyle#12737

According to the https://docs.oracle.com/javase/specs/jls/se17/html/jls-8.html#jls-8.8.9 The default constructor has the same access modifier as the class.
and according to the check https://checkstyle.org/config_design.html#FinalClass a class that has only private constructors and has no descendant classes is declared as final.

@Kevin222004
Copy link
Contributor Author

I have given information on this changes in description.

can we have your valuable feedback on this changes.

Copy link
Contributor

@danieldietrich danieldietrich left a comment

Choose a reason for hiding this comment

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

Thank you! This looks good to me

@danieldietrich danieldietrich merged commit 7a186d8 into vavr-io:master Apr 20, 2023
@Kevin222004
Copy link
Contributor Author

@danieldietrich thanks for reviewing but can give your feedback on this changes usually many people avoid this what is your opinion on this type of changes

@ezoerner
Copy link

I'll throw in my two cents. Personally and in general, I'm a fan of declaring "effectively final" entities as explicitly final, but I think this is most important for variables (for immutable data/references) and less important for classes. Also, I think it's even less important for test classes to be declared final, and yet again even less important for inner classes in tests to be declared final. So sure, I think the checkstyle Final Class check is somewhat useful and good to have, but I don't think its application to these particular test classes in vavr is very important at all—but it doesn't hurt anything, either.

@danieldietrich
Copy link
Contributor

Yes, that was also my impression.

The test code does not need to be checkstyle conform. IMO people overdo checkstyle a bit in real world projects, the cost/income ratio isn't good. it leads to infinite team discussions about minor "improvements". there are rarely major or critical issues found.

During the years I got more relaxed on private implementation style. The public API and the overall design are important to me. Having general rules is good. Most of the time, looking at existing code and having proper code reviews is sufficient.

In the particular case of your change I also thought: "it does not hurt".

Thanks.

@romani
Copy link

romani commented Apr 21, 2023

Thanks a lot for feedback. This is why we created this PR to get feedback.
I also had exactly same worries that over focus on placing final on all is too much. But we have big set of users who want to be super explicit and we try to reconcile user demands from checkstyle behavior by extra options to allow be strict on level that team needs. We are not asking user why they need such demands from code, we just highlighting to user what we were asked to do in config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants