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 #5608: Adding support for allowedAnnotations in javadocTypeCheck #6090

Merged
merged 1 commit into from Nov 14, 2018

Conversation

@ngeor
Copy link
Contributor

ngeor commented Aug 18, 2018

Issue: #5608

Copied the example of javadocMethod. Moved code to util class to avoid duplication. Added tests.

@romani

This comment has been minimized.

Copy link
Member

romani commented Aug 18, 2018

At circle CI I see "Your current usage represents 1211% of your checkstyle Linux plan's limit. Please upgrade in order to ensure no disruption in building." We need to figure out what to do.

@ngeor

This comment has been minimized.

Copy link
Contributor Author

ngeor commented Aug 18, 2018

That sounds bad. I wonder however why "ci/circleci: build-caches" passes.

@ngeor

This comment has been minimized.

Copy link
Contributor Author

ngeor commented Aug 24, 2018

Good afternoon @romani
Are there any news on this problem?
Best regards,
Nikolaos

@romani

This comment has been minimized.

Copy link
Member

romani commented Aug 29, 2018

please rebase on latest mastar:HEAD , I just merged fix for circle-ci

@ngeor ngeor force-pushed the ngeor:5608-javadoctype-allowedannotations branch from f747f63 to 7913ea1 Aug 29, 2018
@ngeor

This comment has been minimized.

Copy link
Contributor Author

ngeor commented Aug 29, 2018

Hi @romani I managed to get a green build. Looking forward to your code review!

@romani

This comment has been minimized.

Copy link
Member

romani commented Sep 5, 2018

@ngeor , please squash 2 commits in one.

Copy link
Member

romani left a comment

items to improve:

src/xdocs/config_javadoc.xml Outdated Show resolved Hide resolved
@ngeor ngeor force-pushed the ngeor:5608-javadoctype-allowedannotations branch from 0f77f41 to 5fd63c5 Sep 5, 2018
boolean result = false;
final DetailAST annotationHolderNode = getAnnotationHolder(ast);
DetailAST annotationNode = annotationHolderNode.findFirstToken(TokenTypes.ANNOTATION);
while (annotationNode != null && annotationNode.getType() == TokenTypes.ANNOTATION) {

This comment has been minimized.

Copy link
@rnveach

rnveach Sep 5, 2018

Member

annotationNode.getType() == TokenTypes.ANNOTATION

I don't think you can have this here, since Javadocs/comments sometimes get placed inside the modifiers section next to annotations.
Add some javadocs to your tests to confirm.

This comment has been minimized.

Copy link
@ngeor

ngeor Sep 6, 2018

Author Contributor

I moved this code from its original location, so I have not changed this at all.
Could you please give me an example of what you mean so I can add the test?

This comment has been minimized.

Copy link
@romani

romani Sep 9, 2018

Member

@rnveach , please reply.

This comment has been minimized.

Copy link
@ngeor

ngeor Sep 9, 2018

Author Contributor

pending reply from @rnveach

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Sep 6, 2018

@ngeor CI is failing. Please run mvn verify test before pushing to verify all is good as it will test most of the CI. Please keep number of commits as 1.

@ngeor

This comment has been minimized.

Copy link
Contributor Author

ngeor commented Sep 6, 2018

@rnveach yes I know it is failing. It is because I did not add unit tests on a refactoring I did. I would like to have first your approval before I proceed with the tests, otherwise I spend more time on this and if you don't like it then I will have to throw it away. The refactoring is all inside AnnotationUtil.

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Sep 6, 2018

@ngeor Then please make that request clear after pushing changes.
We usually don't continue reviewing the PR until CI passes.

@ngeor ngeor force-pushed the ngeor:5608-javadoctype-allowedannotations branch 2 times, most recently from 3dec686 to 7aa238e Sep 6, 2018
@ngeor

This comment has been minimized.

Copy link
Contributor Author

ngeor commented Sep 8, 2018

It seems that when I squash my commits into one, GitHub loses our conversation. I'll leave the comment here and I hope you see it. The build is green again, but it would be interesting to discuss that last point: #6090 (comment)

If you have any tips on how to push to GitHub without confusing the conversation context, please let me know :)

@romani

This comment has been minimized.

Copy link
Member

romani commented Sep 9, 2018

It seems that when I squash my commits into one, GitHub loses our conversation

it does not loose a review discussion.
Please reply to EACH point that we raised as "DONE" or some other comment.

@romani

This comment has been minimized.

Copy link
Member

romani commented Sep 9, 2018

@rnveach , as started review first - please finish review first, I will do final review.

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Sep 9, 2018

@romani See #6090 (comment) for question on logic.
and #6090 (comment) for separate question.

I don't really see anything else and am basically done my review.

Copy link
Member

romani left a comment

while we are in final stage of discussion over a code please prepare regression diff report for this Check.
https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#diffgroovy-diff-report-generation
Do not hesitate is ask questions.

items to improve:

@@ -9,7 +9,7 @@ function checkPitestReport() {
SEARCH_REGEXP="(span class='survived'|class='uncovered'><pre>)"
grep -irE "$SEARCH_REGEXP" target/pit-reports \
| sed -E 's/.*\/([A-Za-z]+.java.html)/\1/' | sort > target/actual.txt
printf "%s\n" "${ignored[@]}" | sed '/^$/d' > target/ignored.txt
printf "%s\n" "${ignored[@]}" | sed '/^$/d' | sort > target/ignored.txt

This comment has been minimized.

Copy link
@romani

romani Sep 9, 2018

Member

please explain this change

This comment has been minimized.

Copy link
@ngeor

ngeor Sep 9, 2018

Author Contributor

When I run the mutation tests locally on my laptop, I get random failures. I noticed then that in this script you are sorting "target/actual.txt" but not "target/ignored.txt". After I applied sorting on "target/ignored.txt", my problems were gone. To me it felt logical that if you sort one part of the comparison, the other part should also be sorted, it's more symmetrical this way.

This comment has been minimized.

Copy link
@romani

romani Sep 13, 2018

Member

@ngeor ,
interesting point ..... "target/ignored.txt" suppose to be sorted, to keep diff on ignored under easy control.
Please try to revert this change(try one more time) and share details in separate issue.

This comment has been minimized.

Copy link
@ngeor

ngeor Sep 13, 2018

Author Contributor

Sure thing I will give it a try.

}

return annotations.contains(identNode.getText());
}) != null;

This comment has been minimized.

Copy link
@romani

romani Sep 9, 2018

Member

this is impossible to read properly.
please store result of getAnnotation in some variable to reuse in "result = annotation != null"

This comment has been minimized.

Copy link
@ngeor

ngeor Sep 9, 2018

Author Contributor

@romani I appreciate the time you're taking to review my PR (and the same applies for @rnveach of course). I do however need to point out that on a few cases your remarks might be rephrased in a friendlier tone, without weakening their objective value.

Example 1: this is hard to read properly (hard instead of impossible which can be perceived as too negative)
Example 2: please reply to each comment (each instead of EACH which can be perceived as too angry)

Written medium does not allow for carrying over the tone of voice and of course different cultures perceive things differently.

@ngeor ngeor force-pushed the ngeor:5608-javadoctype-allowedannotations branch from 7aa238e to b636441 Sep 9, 2018
@ngeor

This comment has been minimized.

Copy link
Contributor Author

ngeor commented Sep 9, 2018

I ran the following command for the diff report:

groovy .\diff.groovy --localGitRepo ..\..\checkstyle\ --baseBranch upstream/master \
--patchBranch 5608-javadoctype-allowedannotations --config my_check.xml \
--listOfProjects projects-to-test-on.properties

It completed successfully and created some files in the reports folder but I don't see anything interesting in them (hopefully that's a good sign).

reports/diff/index.html:

WARNING: Excludes are ignored by diff.groovy.
Base branch: upstream/master
Base branch last commit SHA: e2f7bb6
Base branch last commit message: "minor: fixed inlined javadoc tag from being split between lines"

Patch branch: 5608-javadoctype-allowedannotations
Patch branch last commit SHA: b636441
Patch branch last commit message: "Issue #5608: Adding support for allowedAnnotations in javadocTypeCheck"

And then one more file reports/diff/guava/index.html which says there are 0 violations.

Is there a way to submit these reports or attach them somewhere?

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Sep 12, 2018

Is there a way to submit these reports or attach them somewhere?

See https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#deploy-report

@ngeor

This comment has been minimized.

Copy link
Contributor Author

ngeor commented Sep 12, 2018

Thanks @rnveach -- there is however one open question I'm waiting for your help. I should probably wait first for that answer, in case it leads to followup work. (this question here #6090 (comment) )

Edit: I uploaded the report here https://ngeor.github.io/checkstyle-reports/

@ngeor ngeor force-pushed the ngeor:5608-javadoctype-allowedannotations branch from dfcf995 to 66d19e7 Sep 16, 2018
@ngeor

This comment has been minimized.

Copy link
Contributor Author

ngeor commented Sep 17, 2018

I uploaded the report here https://ngeor.github.io/checkstyle-reports/

Please expand testing to all projects listed in https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/projects-to-test-on.properties . Sometimes if there is an issue, only 1 or 2 projects will show it.

I had used that file projects-to-test-on.properties when I run the command the first time. The mention of the filename projects-to-test-on.properties is present in the command line I had provided.

Are you suggesting I should modify this file and un-comment all these repositories and then run the report? (and then why are all the projects commented-out in the first place)

On a side comment, these checks need to become part of the CI pipeline.

@ngeor

This comment has been minimized.

Copy link
Contributor Author

ngeor commented Sep 17, 2018

I have update the report with a few more projects, namely:

protonpack
RxJava
apache-jsecurity
pmd
lombok-ast
guava-mvnstyle
sevntu-checkstyle
sevntu-checkstyle-with-excludes
android-launcher
Vavr
Orekit
jOOL
apache-ant
elasticsearch
MaterialDesignLibrary
java-design-patterns
nbia-dcm4che-tools
spotbugs
spotbugs-with-excldues
apache-struts
checkstyle
guava
Hbase

To complete the report with all the projects, I need to create a virtual machine with Linux. The repositories that are missing could not be checked out locally on Windows because the path name was too long and git checkout fails. Also I don't have mercurial installed.

I hope that this is what you meant.

@ngeor ngeor force-pushed the ngeor:5608-javadoctype-allowedannotations branch from 66d19e7 to 7bc2065 Sep 23, 2018
@ngeor

This comment has been minimized.

Copy link
Contributor Author

ngeor commented Sep 23, 2018

I just managed to run the checkstyle-tester report inside a Linux VM without any errors. The report is uploaded here https://ngeor.github.io/checkstyle-reports/
Looking forward to your feedback.

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Oct 2, 2018

@ngeor Sorry for long delay.

https://ngeor.github.io/checkstyle-reports/

You didn't change the configuration to run the regression with.
https://ngeor.github.io/checkstyle-reports/hibernate-orm/index.html
It is still looking at ThrowsCount and not JavadocMethodCheck. You must update https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/my_check.xml#L26 with the check and properties you are testing with.

@ngeor

This comment has been minimized.

Copy link
Contributor Author

ngeor commented Oct 3, 2018

Hi @rnveach

Thank you for the reply.

I will give it a try with the my_check.xml. I did not know I had to edit this file.

I should probably add not only JavadocMethodCheck but also JavadocTypeCheck which is what my PR is primarily about.

Best regards,
Nikolaos

@ngeor

This comment has been minimized.

Copy link
Contributor Author

ngeor commented Oct 3, 2018

I just uploaded a fresh set of reports. I added the JavadocMethod and JavadocType checks, in addition to the pre-existing ThrowsCount.

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Oct 5, 2018

@ngeor Thanks for the report. It shows there are no changes in original functionality.

@rnveach
rnveach approved these changes Oct 5, 2018
@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Oct 5, 2018

@romani Please finish your review.

@ngeor ngeor force-pushed the ngeor:5608-javadoctype-allowedannotations branch from 7bc2065 to fb5fff4 Oct 15, 2018
@ngeor

This comment has been minimized.

Copy link
Contributor Author

ngeor commented Oct 15, 2018

Hi all,
I noticed that something was merged, so I changed the documentation on my PR to report that it applies from version 8.14 onwards.
Unfortunately, this caused one build to error, which I think is a random error.
Could someone please restart the build?
Also, I'm looking for a final sign off from @romani
Best regards,
Nikolaos

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Oct 15, 2018

CI is green now.

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Oct 27, 2018

@romani ping

@ngeor

This comment has been minimized.

Copy link
Contributor Author

ngeor commented Nov 5, 2018

@romani any news on this? Thanks.

Copy link
Member

romani left a comment

Sorry for delay in reply.
Minor update is required:

<td><a href="property_types.html#stringSet">String Set</a></td>
<td><code>{}</code></td>
<td>8.14</td>
</tr>

This comment has been minimized.

Copy link
@romani

romani Nov 10, 2018

Member

Please append some example of usage of new option for code on spring annotations as described in issue.
I think it is reasonable to make same valuable default value for some jdk annotations like https://docs.oracle.com/en/java/javase/11/docs/api/java.compiler/javax/annotation/processing/Generated.html or ...

This comment has been minimized.

Copy link
@ngeor

ngeor Nov 11, 2018

Author Contributor

Hi @romani ,
I did the following changes:

  • changed "since" to 8.15 as I see you've already released 8.14
  • made @Generated the default value
  • added an example in the documentation for a few Spring annotations
  • adjusted unit tests to verify the new default behavior regarding @Generated
  • rebased on top of master, squashed in a single commit
@ngeor ngeor force-pushed the ngeor:5608-javadoctype-allowedannotations branch 2 times, most recently from 09d6464 to 01005d8 Nov 11, 2018
Copy link
Member

romani left a comment

thanks a lot for quick reply.

More items to improve:

* @param predicate The predicate which decides if an annotation matches
* @return the AST representing that annotation
*/
private static DetailAST getAnnotation(final DetailAST ast,

This comment has been minimized.

Copy link
@romani

romani Nov 11, 2018

Member

too much getAnnotation methods.
please rename it to "findAnnotation", as it actually do search.

This comment has been minimized.

Copy link
@ngeor

ngeor Nov 11, 2018

Author Contributor

fixed


boolean result = false;

if (annotations != null && !annotations.isEmpty()) {

This comment has been minimized.

Copy link
@romani

romani Nov 11, 2018

Member

annotations != null

this is util method, so we can demand that arguments are not null, please make validation for this the same as ast == null above.

This comment has been minimized.

Copy link
@ngeor

ngeor Nov 11, 2018

Author Contributor

fixed

boolean result = false;

if (annotations != null && !annotations.isEmpty()) {
final DetailAST firstMatchingAnnotation = getAnnotation(ast, annotationNode -> {

This comment has been minimized.

Copy link
@romani

romani Nov 11, 2018

Member

firstMatchingAnnotation

you do not have list of annotations , so make variable name simply "annotation"

This comment has been minimized.

Copy link
@ngeor

ngeor Nov 11, 2018

Author Contributor

firstMatchingAnnotation is a better name because it tells the developer who'll read the code that if there are multiple annotations in the annotations list, only the first one will be returned.

This comment has been minimized.

Copy link
@romani

romani Nov 11, 2018

Member

Ok, please rename getAnnotation to mention that you return only first found item.

This comment has been minimized.

Copy link
@ngeor

ngeor Nov 12, 2018

Author Contributor

It was already renamed to findAnnotation as per your earlier comment, I will rename it to findFirstAnnotation

@@ -239,7 +253,8 @@ private boolean shouldCheck(final DetailAST ast) {
&& (excludeScope == null
|| !customScope.isIn(excludeScope)
|| surroundingScope != null
&& !surroundingScope.isIn(excludeScope));
&& !surroundingScope.isIn(excludeScope))
&& !AnnotationUtil.containsAnnotation(ast, allowedAnnotations);

This comment has been minimized.

Copy link
@romani

romani Nov 11, 2018

Member

if user defined allowed annotation in fully qualified form at property allowedAnnotations (to be certain what annotation consider as allowance, to avoid mismatch if there is name conflict) but will use in annotation in short form in code.

How it is supported by this code ?

Please update UT to have such cases.
Update documentation to clearly define what matching will be is user specify only short names in allowedAnnotations, and what behavior will be in case of definition by fully-qualified names.

This comment has been minimized.

Copy link
@ngeor

ngeor Nov 11, 2018

Author Contributor

There's already FQN in my unit test.

This comment has been minimized.

Copy link
@romani

romani Nov 11, 2018

Member

Ok, I might miss it, I will review again.
But users will not read it, please update documentation.

Update: I do not see it ut with fully qualified name in property, please send me direct link to it.

This comment has been minimized.

Copy link
@ngeor

ngeor Nov 12, 2018

Author Contributor

Sorry, you meant in the configuration of the check itself. I did not read your comment correctly.

I only have the case where the code under inspection defines a FQN for the annotation. I will add the corresponding test where the check defines a FQN and will update the docs.

</p>
<source>
&lt;module name="JavadocType"&gt;
&lt;property name="allowedAnnotations" value="SpringBootApplication,Configuration"/&gt;

This comment has been minimized.

Copy link
@romani

romani Nov 11, 2018

Member

please add example of java file and by trailing single line comment show user where violation will be.
See examples at https://checkstyle.org/config_annotation.html#AnnotationLocation_Examples

It is very user friendly to show config and how it will work in the same time.

This comment has been minimized.

Copy link
@ngeor

ngeor Nov 11, 2018

Author Contributor

I think that my example is good enough. It is actually better than the check JavadocMethod (which is what I based my changes on), which doesn't even have an example like mine (and it's already on production) http://checkstyle.sourceforge.net/config_javadoc.html#JavadocMethod

This comment has been minimized.

Copy link
@romani

romani Nov 11, 2018

Member

I think that my example is good enough.

Should be enhanced.

It is actually better than the check JavadocMethod (which is what I based my changes on), which doesn't even have an example like mine (and it's already on production)

Sorry that we have bunch of bad examples in documentation. All new stuff we demand do match best practices that we know now.

We are welcome all to come and fix documentation to match best practices. To ease reading of it and quickly understand how each options works. For now we update only items that come as new content.

This comment has been minimized.

Copy link
@ngeor

ngeor Nov 12, 2018

Author Contributor

I'm just wondering if we're falling for the trap of the better being the enemy of the good. As a checkstyle user, I never had a problem understanding the checkstyle documentation. But maybe that's just me.

I've updated the docs with side by side example of source code and matching checkstyle configuration.

This comment has been minimized.

Copy link
@romani

romani Nov 13, 2018

Member

I hope you enjoying checkstyle documentation, we improved it a lot during few past year, and work is still in progress...
But I am working not only with a lot of engineers, and not all of them very experienced, so come to such practices in documentation due a lot of requests to clarify or explain.

I want to make docs to be clear for all. After discussion in issue tracker and classifying issue as won't fix or ... We usually update doc to avoid such questions in future

This comment has been minimized.

Copy link
@romani

romani Nov 13, 2018

Member

I hope you enjoying checkstyle documentation, we improved it a lot during few past year, and work is still in progress...
But I am working not only with a lot of engineers, and not all of them very experienced, so come to such practices in documentation due a lot of requests to clarify or explain.

I want to make docs to be clear for all. After discussion in issue tracker and classifying issue as won't fix or ... We usually update doc to avoid such questions in future

This comment has been minimized.

Copy link
@ngeor

ngeor Nov 13, 2018

Author Contributor

What I would personally like to see is a collection of popular presets and somehow sorted by the number of downloads. I have created my own set of rules, which I don't like doing. I'd rather follow a popular preset, if it's close to my taste. Something like what ESLint is doing.

The documentation is great. What I think might improve it is if the core team of checkstyle gets a bit more diverse. I'm always trying to make sure the documentation I write is in proper English. I'm not a native English speaker, so I make mistakes, like everybody. When all developers have the same background, it's difficult for them to recognize their own grammar mistakes, so diversity helps there.

This comment has been minimized.

Copy link
@romani

romani Nov 14, 2018

Member

What I would personally like to see is a collection of popular presets and somehow sorted by the number of downloads.

if you know how to do this, please create issue and send us PR :) .

What I think might improve it is if the core team of checkstyle gets a bit more diverse.

Please be welcome to be part of team :) .

When all developers have the same background, it's difficult for them to recognize their own grammar mistakes, so diversity helps there

I try attract more and more people to project. There are some web services that can help us with localization - #6191.
Here is how I try to verify translations now - #6067 (comment)
If you know better way - let us know.

This comment has been minimized.

Copy link
@ngeor

ngeor Nov 14, 2018

Author Contributor

Thank you! I'll try to contribute something else in the future. I think I'd like to do the code cleanup we discussed that all test methods should start with test, which currently isn't the case.

This comment has been minimized.

Copy link
@romani

romani Nov 14, 2018

Member

Please create and issue on this to discuss details

@ngeor ngeor force-pushed the ngeor:5608-javadoctype-allowedannotations branch 3 times, most recently from b1b1926 to 0fd82e7 Nov 11, 2018
Copy link
Member

romani left a comment

Item to improve:

<source>
@SpringBootApplication // no violations about missing comment on class
public class Application {}
[..]

This comment has been minimized.

Copy link
@romani

romani Nov 13, 2018

Member

Please remove this, we try to keep examples compilable, ready for copy paste and run

This comment has been minimized.

Copy link
@ngeor

ngeor Nov 13, 2018

Author Contributor

fixed

@ngeor ngeor force-pushed the ngeor:5608-javadoctype-allowedannotations branch from 0fd82e7 to 80ed9ff Nov 13, 2018
@romani
romani approved these changes Nov 14, 2018
@romani romani merged commit 20e7c59 into checkstyle:master Nov 14, 2018
16 checks passed
16 checks passed
IDEA Inspections Pull Request (Checkstyle) TeamCity build finished
Details
Shippable Run 8096 status is SUCCESS.
Details
ci/circleci: build-caches Your tests passed on CircleCI!
Details
ci/circleci: pitest1 Your tests passed on CircleCI!
Details
ci/circleci: pitest2 Your tests passed on CircleCI!
Details
ci/circleci: pitest3 Your tests passed on CircleCI!
Details
ci/circleci: pitest4 Your tests passed on CircleCI!
Details
ci/circleci: pitest5 Your tests passed on CircleCI!
Details
ci/circleci: pitest6 Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to 7f663da
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins Build succeeded
Details
security/snyk - pom.xml (romani) No new issues
Details
wercker/build Wercker pipeline passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.