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

Issue #12520: removes missing package-info Javadoc check in JavadocStyle #12521

Merged
merged 1 commit into from Dec 30, 2022

Conversation

rnveach
Copy link
Member

@rnveach rnveach commented Dec 12, 2022

Issue #12520

Regression: http://rveach.no-ip.org/checkstyle/regression/319/

Please note, configuration was created to show that with the functionality removed is exactly the same as MissingJavadocPackage. The extra properties added were to allow regression to see them as the same to remove duplicates and trim the report.

http://rveach.no-ip.org/checkstyle/regression/319/checkstyle/index.html#A1
Is an example of a false negative as the old check was not identifying javadocs in the correct location.

http://rveach.no-ip.org/checkstyle/regression/319/checkstyle/index.html#A2
Is an example of a false positive as the new check only verifies the package has a styled javadoc. This was only being picked up by JavadocStyle as it saw there was no javadoc and it was in a package-info file, which it probably shouldn't have been doing. Basically the check if looking at all token types in the file even if they weren't a package definition. There are other "missing" checks which should handle this case, so there should be nothing lost in the end.


property still in use by other Checks

~/java/github/romani/checkstyle [master|✔] 
$ ag -Q "javadoc.missing\""
src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocStyleCheck.java
308:    public static final String MSG_JAVADOC_MISSING = "javadoc.missing";

src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocVariableCheck.java
184:    public static final String MSG_JAVADOC_MISSING = "javadoc.missing";

src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/MissingJavadocTypeCheck.java
191:    public static final String MSG_JAVADOC_MISSING = "javadoc.missing";

src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/SummaryJavadocCheck.java
236:    public static final String MSG_SUMMARY_JAVADOC_MISSING = "summary.javaDoc.missing";

src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/MissingJavadocMethodCheck.java
270:    public static final String MSG_JAVADOC_MISSING = "javadoc.missing";

src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/MissingJavadocPackageCheck.java
80:    public static final String MSG_PKG_JAVADOC_MISSING = "package.javadoc.missing";

@rnveach rnveach force-pushed the issue_7416_1 branch 3 times, most recently from 8b578e0 to 2c02c7a Compare December 13, 2022 00:34
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

https://checkstyle.org/config_javadoc.html#MissingJavadocPackage

Checks for missing Javadoc comments in package-info.java files.

we need to update description to mention that this Check does only package javadoc. If any type are declared in such file in addition, they will not be validated.

But in this case validation of other types in package-info.java should be on JavadocStyle. can we keep this easily ? or we can create separate issue to cover this very rare usage of code.

items:

@rnveach
Copy link
Member Author

rnveach commented Dec 13, 2022

we need to update description to mention that this Check does only package javadoc. If any type are declared in such file in addition, they will not be validated.

Done.

But in this case validation of other types in package-info.java should be on JavadocStyle. can we keep this easily ? or we can create separate issue to cover this very rare usage of code.

Please see my change to the package-info as part of JavadocStyle's test. It is still validating the file, and everything it was before, it is just specifically removing the check if a javdoc is missing.
If we were dropping normal styling check, regression would show more differences as you would see dropped styling and MissingJavadocPackage will not pick it up.

@rnveach rnveach force-pushed the issue_7416_1 branch 2 times, most recently from ac04040 to de4ab04 Compare December 13, 2022 15:22
@rnveach
Copy link
Member Author

rnveach commented Dec 13, 2022

https://app.circleci.com/pipelines/github/checkstyle/checkstyle/16604/workflows/083adf23-1c23-4b3c-92af-f2525931c94b/jobs/216874
CircleCI is having issues.

error checking out branch: object not found

I already repushed and rebased on master.

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Items

@rnveach
Copy link
Member Author

rnveach commented Dec 20, 2022

@nrmancuso ping
https://github.com/checkstyle/checkstyle/actions/runs/3742296373/jobs/6353051245#step:7:15

Unnecessary suppressed mutation(s) found and should be removed:
removed call to com/puppycrawl/tools/checkstyle/grammar/java/JavaLanguageLexer::removeErrorListeners

I remember an issue of this. (#12186)

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

ok to merge

@nrmancuso
Copy link
Member

Github, generate site

@nrmancuso
Copy link
Member

rebased on latest master

@nrmancuso
Copy link
Member

Github, generate site

@github-actions
Copy link
Contributor

@nrmancuso
Copy link
Member

Restarted pitest-common, looks like random failure.

@nrmancuso
Copy link
Member

Al CI failures are known (Travis and XML issues)

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.

None yet

3 participants