Issue #2427: added customizable javadoc tokens

[rnveach]

Issue #2427.

Added acceptable and required javadoc tokens. Code was mostly copied from TreeWalker which this class is mimicking.
All javadoc checks required getRequiredJavadocTokens to be added.

Questions:
1)
The issue made note of an area that had documentation on this already but was removed.
Was that documentation valid that the 2 tokens are optional?
Are there any other optional tokens for the javadoc checks? I didn't see anything jump out when glancing over.

Some things to note:
1)
We needed some place to validate that the javadoc tokens set for the check are correct and require us to be able to throw a CheckstyleException. I did this in init and so had to add the exception to the throws clause, and this is why there is an update in FallThroughCheck. I don't see any other place we can do this.
I brought this up before where none of the AbstractCheck methods allow throwing an exception, even CheckstyleException and I don't think we should use runtime exceptions.
If your ok with it, I think we should change all the methods to be able to throw CheckstyleExceptions.

Javadoc's walk method changed slightly because I think there is a bug with waitsForProcessing and when we loop while (curNode != null && toVisit == null) { more than once since the value isn't updated.
I will try to find a test to show this and add it.

AtClauseOrderCheck changed slightly to not loop through the children, but to use the visit token's power. This would be helpful if the token appears in other places than a child of the root.

[codecov-io]

Current coverage is 100% (diff: 100%)

Merging #3512 into master will not change coverage

@@           master   #3512   diff @@
=====================================
  Files         273     273          
  Lines       13074   13170    +96   
  Methods         0       0          
  Messages        0       0          
  Branches     2978    3004    +26   
=====================================
+ Hits        13074   13170    +96   
  Misses          0       0          
  Partials        0       0          

Powered by Codecov. Last update dda0b9e...ad58563

[romani]

Was that documentation valid where the 2 tokens are optional?

I doubt it was valid. It was imaginary config.

Are there any other optional tokens for the javadoc checks?

We do not have. We have only 6 Checks that extends AbstractJavadocCheck:
AtclauseOrderCheck
JavadocParagraphCheck
JavadocTagContinuationIndentationCheck
NonEmptyAtclauseDescriptionCheck
SingleLineJavadocCheck
SummaryJavadocCheck

All of them were created to cover Google style, right after ANTLR grammar was introduced. Almost all of them are simple Checks. Only NonEmptyAtclauseDescriptionCheck could have some reasons to be customized. But it was missed from our attention during implementation as no customization was required for Google style.

[romani]

even CheckstyleException and I don't think we should use runtime exceptions.

we should not pollute method signatures with checked exceptions internally.
Checked exception give no extra details in comparison to not specifying it at all. It is already clear that there is possibility of exception in code. That is why non of Checks have it - when can throw all possible exceptions and TreeWalker convert them to CheckstyleException to deliver to outside of library as some exception that happened in checkstyle to easily let main code distinguish it and process.
I would use IllegalStateException , to let it fly to TreeWalker as any other exception in Checks.

I will try to find a test to show this and add it.

Such unclear fixes should beeasily caught by cobertura. If you do not have problem, that is strange.

AtClauseOrderCheck changed slightly to not loop through the children, but to use the visit token's power. This would be helpful if the token appears in other places than a child of the root.

hmmm, it is always a decision point to make Check stateless or stateful (you added a field - it become stateful). I always guided other to make Check stateless is there is no performace hit by it in comparison to statefull implementation.
In the future: Stateless Checks will be marked by us and TreeWalker will run them in MT mode, stateful Checks will run as they run right now.

[rnveach]

I doubt it was valid. It was imaginary config.
We do not have.

Then I will leave validating javadoc tokens till later, as any code I write for it now will remain untested until that time.

I reverted changes to AtClauseOrderCheck.

Stateless Checks will be marked by us and TreeWalker will run them in MT mode, stateful Checks will run as they run right now.

I will start an email chain discussing this with you. :)

we should not pollute method signatures with checked exceptions internally.

CheckstyleException is ours, so I don't really consider it polluting by adding it. My main reason for wanting it there is because I dislike wrapping exception upon exception multiple times making the final printed exception overly complex with Caused by. But I am also unsure when something should and shouldn't be a runtime exception.
I reverted the changes and used IllegalStateException.

Such unclear fixes should be easily caught by cobertura. If you do not have problem, that is strange.

Cobertura just checks for dead code. It doesn't check for logic errors, which this is. Just because a method runs and returns a result, doesn't mean it returns the correct result.
I added the check testVisitLeaveToken to show the issue. If you comment out these 3 new lines in AbstractJavadocCheck:

                 if (toVisit == null) {
                     curNode = curNode.getParent();
+                    if (curNode != null) {
+                        waitsForProcessing = shouldBeProcessed(curNode);
+                    }

you will get more calls to leaveJavadocToken than to visitJavadocToken. They should be 1 to 1. The reason for the issue is waitsForProcessing is a variable with a value based on curNode. When curNode changes, waitsForProcessing should too, but it wasn't.
Without these lines, the test will fail with: AssertionError: expected:<406> but was:<464>

[romani]

I dislike wrapping exception upon exception multiple times making the final printed exception overly complex with Caused by

yes, but unpleasant code is more often read by engineers.

Code looks good.

please update http://checkstyle.sourceforge.net/writingjavadocchecks.html#Customize_token_types_in_Javadoc_Checks and we good to merge.

[rnveach]

@romani Done.

[romani]

please address few code review points

[romani]

hmm, the whole reason of the issue was to customize NonEmptyAtclauseDescription , but with such method we do not allow customization.
required should be empty array.

[rnveach]

the whole reason of the issue was to customize NonEmptyAtclauseDescription

I took your reply here to mean there were no optional tokens for any checks.

Was that documentation valid where the 2 tokens are optional?

I doubt it was valid. It was imaginary config.

Are there any other optional tokens for the javadoc checks?

We do not have.

Done in next update.

[romani]

there should be getAcceptableJavadocTokens usage. We require all.
Please fix in all other cases.

[rnveach]

Is there a reason for this? getAcceptable's default implementation copies getDefault, so in the end it is the same.

[rnveach]

Done.

[rnveach]

@romani Changes were addressed.
Allowing optional javadoc tokens in NonEmptyAtclauseDescriptionCheck required me to implement this code in XDocsPagesTest so they would appear in the xdocs.