-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Issue #5411: split JavadocType for missing javadocs #6554
Conversation
36cd9ef
to
06538c5
Compare
This comment has been minimized.
This comment has been minimized.
06538c5
to
e45c59d
Compare
No Exception Regression: http://rveach.no-ip.org/checkstyle/regression/reports/234/ No exceptions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
items to imropve:
.../checkstyle/checks/javadoc/missingjavadoctype/InputMissingJavadocTypeAllowedAnnotations.java
Outdated
Show resolved
Hide resolved
...pycrawl/tools/checkstyle/checks/javadoc/missingjavadoctype/InputMissingJavadocTypeInner.java
Outdated
Show resolved
Hide resolved
...wl/tools/checkstyle/checks/javadoc/missingjavadoctype/InputMissingJavadocTypePublicOnly.java
Outdated
Show resolved
Hide resolved
src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/MissingJavadocTypeCheck.java
Show resolved
Hide resolved
e45c59d
to
40ac389
Compare
Pushed changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last critical item:
src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/MissingJavadocTypeCheck.java
Outdated
Show resolved
Hide resolved
Along with the above changes I also increased the version number of the check because of the release. |
travis failure will be fixed when checkstyle/contribution#365 is merged. |
@romani Feel free to review again. If nothing happens in the next few days I will merge this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
items to discuss:
src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/MissingJavadocTypeCheck.java
Outdated
Show resolved
Hide resolved
* <pre> | ||
* <module name="MissingJavadocType"> | ||
* <property name="scope" value="private"/> | ||
* <property name="excludeScope" value="package"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if target is private only .... what is reason or excludeScope=package ?
smth is missed in description of Check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@romani Maybe you are confused by what scope does? See http://checkstyle.sourceforge.net/property_types.html#scope
Private doesn't mean only private types.
'package' means all 'package', 'protected' and 'public'
So private (with no exclude) would mean private and including everything included by package.
So including private and excluding package, means only private types are checked in this example.
$ cat TestClass.java
public class PublicClass {}
private class PublicClass {} // violation
protected class PublicClass {}
class PackagePrivateClass {}
$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
"-//Puppy Crawl//DTD Check Configuration 1.3//EN"
"https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
<property name="charset" value="UTF-8"/>
<module name="TreeWalker">
<module name="JavadocType">
<property name="scope" value="private" />
<property name="excludeScope" value="package" />
</module>
</module>
</module>
$ java -jar checkstyle-8.18-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:2: Missing a Javadoc comment. [JavadocType]
Audit done.
Checkstyle ends with 1 errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have an issue to break away from scope and use access modifier.
See #3511
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Omh, .... Do you know definition of java scope ? I do not see any of at https://docs.oracle.com/javase/specs/jls/se8/html/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have energy to not use scope ? We should not leak such a weird stuff to public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this issue, I would prefer to just have these PRs split the checks into 2. It would be easier for compatibility as users could just copy existing config and change module name, for the most part. This is why I am doing a direct copy of the original check.
This check came from JavadocType
, so it is already leaking to the public and it and other checks requires the same changes too. It is also not clear from other issue how we would change check to still use anoninner
or if we would break away from it by dropping.
Do you know definition of java scope ?
There are some mentions of scope in the JLS but not directed at what we are discussing here. ( https://docs.oracle.com/javase/specs/jls/se7/html/jls-6.html#jls-6.3 )
Original issue to bring in access modifier: #3675
Original commit for scope class says javadoc checkscope implementation
from 2002.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to explain users what this weird term mean, at least by examples.
Please add a bit more examples or some words to http://checkstyle.sourceforge.net/property_types.html#scope
If we have home made term, lets at least explain it.
Please do not to forget to add "MIGRATION NOTE" to issue after merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/MissingJavadocTypeCheck.java
Show resolved
Hide resolved
src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/MissingJavadocTypeCheck.java
Outdated
Show resolved
Hide resolved
src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/MissingJavadocTypeCheck.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved with minor request to extend general doc for scope property.
in future please do not reuse bad design/terms from old Javadoc checks, they are weird. We need to create better new generation of such set of Checks in right way. Migration will be pain for users no matter what.
Done. |
I am now merging this first. CI should pass after contribution PR is merged. |
Issue #5411
This is for MissingJavadocType. It was pretty much a copy/paste of the original check.