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

New MissingJavadocCheck(s) #5411

Closed
rnveach opened this issue Dec 31, 2017 · 14 comments

Comments

Projects
None yet
3 participants
@rnveach
Copy link
Member

commented Dec 31, 2017

As discussed at #4983 (comment) ,

once we start converting old Java checks using Javadoc regular expression to Javadoc checks using grammar, we lose the functionality of reporting a violation on an item if a Javadoc is missing.

MissingDeprecated has an property skipNoJavadoc which will skip violations if a javadoc is missing on a deprecated method.
JavadocMethodCheck has various properties like minLineCount and allowedAnnotations which will not print a missing javadoc violation if the number of lines in the method are too small or it contains an annotation.

We can't remove these functionalities, so we need to split them into new check(s) that will remain as Java checks. Their only purpose will be to report a violation if a javadoc is needed.

We need to examine all these properties of our Java checks that examine Javadocs and decide how to best to split them out.
Should we have a new check for each type (Class, Method, etc...) or should they all be contained into 1 check?
Which properties do these new check(s) need from our existing checks? We will need to create a complete list.

Once this is done we can complete #4983.


Migration Note

For JavadocType, functionality for validating missing javadocs was split into a new check MissingJavadocType. scope and excludeScope properties were brought to the new check. allowedAnnotations was brought over but renamed to skipAnnotations.

@pbludov

This comment has been minimized.

Copy link
Collaborator

commented Jan 2, 2018

There is also the UnusedImportCheck, which is both a java and a javadoc check.
For now, this is a java check with all the javadoc stuff implemented with the regular expressions.
Perhaps it makes sense to invent a dedicated kind of checks for such cases.

Here is the basic idea:
#2840 (comment)

@romani romani added the approved label Jan 5, 2018

@romani

This comment has been minimized.

Copy link
Member

commented Jan 6, 2018

rules/criteria for reporting violations for Class and Method could be very different.
For class - if it is public, if extends, is abstract, if deprecated ....
For method - if public, if abstract, if deprecated, if size is bigger in lines, if throws, if name is match regexp, if arguments amount above N.

We will need separate Check for this:
MissingJavadocTypeCheck (class, enum, interface)
MissingJavadocMethodCheck (methods)
MissingJavadocVariableCheck (fields)

user should define by properties what is triggering validation.
By default it is good to demand javadoc on deprecated methods/classes.

existing MissingDeprecatedCheck should stay and become javadoc Check to trigger on each javadoc comment, check java annotation, and then search for javadoc tag and do match. New MissingJavadocMethodCheck should force user to write javadoc when annotation is present, to let on next launch trigger execution of MissingDeprecatedCheck. So it 2 launches of checkstyle all cases should be covered.
Validation of 'is a javadoc is missing' should be removed from existing MissingDeprecatedCheck in the same PR we add the new check. This will allow us to run regression with both in the same configuration and prove there are no differences when both are used together. We are basically splitting the functionality of the check into 2.

@rnveach , please share your ideas.

@romani

This comment has been minimized.

Copy link
Member

commented Jan 6, 2018

@rnveach , I squashed all your+my comments in my above post, please review one more time.

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Jan 6, 2018

It looks fine to me.

@pbludov

This comment has been minimized.

Copy link
Collaborator

commented Jan 6, 2018

And MissingJavadocPackageInfoCheck (package-info.java)

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Jan 6, 2018

And MissingJavadocPackageInfoCheck (package-info.java)

JavadocPackageCheck isn't even an AbstractCheck so it doesn't need a Missing version.
It just examines the file's existence and not anything inside it.

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

I am approving this as this was agreed on.

@rnveach rnveach added the approved label Feb 27, 2019

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2019

MissingJavadocTypeCheck (class, enum, interface)
MissingJavadocMethodCheck (methods)
MissingJavadocVariableCheck (fields)

Just for completeness, other tokens needed are: constructors (method), annotation definitions (type), enum constant definition (field), annotation field (field).

We recently added javadoc parsing on packages. I think we should have MissingJavadocPackageCheck only if the file is package-info.java like @pbludov suggested. We should have a missing for every place a javadoc is valid.

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

Changes to JavadocMethod will affect google_checks. None of the other Javadoc checks are in the config.

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 11, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 11, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 12, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 16, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Apr 1, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Apr 2, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Apr 3, 2019

@rnveach rnveach added this to the 8.20 milestone Apr 5, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Apr 5, 2019

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2019

@romani This can't be closed. We only converted 1 check, 1 is still open in PR, and we probably have around 3 left.

@romani romani reopened this Apr 28, 2019

@romani

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

Probably better to create separate issues, as such long issues conflicts with milestones on issue

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

I will do this.

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

Closing this as new issues were created. Let me know if I missed anything.

@rnveach rnveach closed this Apr 29, 2019

@romani

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

It is ok to keep miscellaneous issue in few releases, but all issues that affects users are better to scope in single release, as issues are major part of our knowledge base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.