Issue #7183: add JavadocMissingWhitespaceAfterAsteriskCheck

[0blivious]

Issue #7183

Contribution PR:
checkstyle/contribution#454
checkstyle/contribution#454 (this link was added to satisfy verify-no-exception-configs build item)

Regression report:
https://0blivious.github.io/7183/index.html

Old regression report
https://0blivious.github.io/index.html

Sevntu-checkstyle PR (merged)
sevntu-checkstyle/sevntu.checkstyle#806

Checkstyle PR
#7883

[rnveach]

https://0blivious.github.io/index.html

Please look over report and configure it for the check you are making.
https://0blivious.github.io/apache-jsecurity/index.html
You are not testing regression on ThrowsCount.
https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#configuration-file

[0blivious]

Please look over report and configure it for the check you are making.
https://0blivious.github.io/apache-jsecurity/index.html
You are not testing regression on ThrowsCount.
https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#configuration-file

I have updated the report. The CI is failing as there are other files in CheckStyle that violate this check. Can tell me how to work around this? Thx very much.

[rnveach]

The CI is failing as there are other files in CheckStyle that violate this check. Can tell me how to work around this?

If the violations are correct, you must fix the issues in Checkstyle's code base.

[romani]

Please squash all commits in one and resolve All CIs, all CIs should be green.
If you have problems let us know

[0blivious]

Please squash all commits in one and resolve All CIs, all CIs should be green.
If you have problems let us know

Hi before I correct all the missing whitespace in CheckStyle code I would like to check, should this check apply to single line comment?

/**This ok or violation? */

[rnveach]

@0blivious Why wouldn't the check apply to all javadocs and all asterisks in the javadoc? What would you want it to do?

[pbludov]

Hi before I correct all the missing whitespace in CheckStyle code I would like to check, should this check apply to single line comment?

Yes, this should be violation.

[pbludov]

From https://0blivious.github.io/apache-struts/xref/nus/sem6/contribution/checkstyle-tester/repositories/apache-struts/core/src/main/java/com/opensymphony/xwork2/util/ResolverUtil.java.html#L58

58   *<pre>
59   *ResolverUtil&lt;ActionBean&gt; resolver = new ResolverUtil&lt;ActionBean&gt;();
60   *resolver.findImplementation(ActionBean.class, pkg1, pkg2);
61   *resolver.find(new CustomTest(), pkg1);
62   *resolver.find(new CustomTest(), pkg2);
63   *Collection&lt;ActionBean&gt; beans = resolver.getClasses();
64   *</pre> 

The violation occurs only on the first line.

@0blivious, please find out what is the output of the Javadoc tool for this class? And DetailAst generated with checkstyle -J
See reference here.

[0blivious]

58   *<pre>
59   *ResolverUtil&lt;ActionBean&gt; resolver = new ResolverUtil&lt;ActionBean&gt;();
60   *resolver.findImplementation(ActionBean.class, pkg1, pkg2);
61   *resolver.find(new CustomTest(), pkg1);
62   *resolver.find(new CustomTest(), pkg2);
63   *Collection&lt;ActionBean&gt; beans = resolver.getClasses();
64   *</pre> 

The violation occurs only on the first line.

I have changed the check implementation and updated the regression report. It now will report violation for all these lines. I got some new problem regarding the implementation with comment like those in https://0blivious.github.io/Hbase/index.html

    /*********
     * State
     *********/

The DetailAst will be

JAVADOC -> JAVADOC
|--LEADING_ASTERISK -> *
|--TEXT ->  ******
|--NEWLINE -> \n
|--LEADING_ASTERISK -> *
|--TEXT ->  State
|--NEWLINE -> \n
|--LEADING_ASTERISK -> *
|--TEXT ->  *******
|--EOF -> <EOF>

The current visitJavadocToken will report violation for the 1st and 3rd line. Should we tolerate this type of comment?

Also can I get some help with the pitest2? I am not sure why it fails......Thx in advance!

[pbludov]

/*********
 * State
 *********/

There should be no violation. The html generated by the Javadoc tool is State (no asterisks at all).
Leading asterisks are tricky and processed in non obvious way.
The specs says that there may be many leading asterisks on the line. Our Javadoc parser is more strict: all asterisks except the first one are treated as a text. So, you have to process the text node after the leading asterisk and skip all * on the front. For the case

|--LEADING_ASTERISK -> *
|--TEXT ->  ******
|--NEWLINE -> \n

it should be same as

|--LEADING_ASTERISK -> *
|--NEWLINE -> \n

May be this check should be a Java check, not Javadoc check, as this one.

[pbludov]

@0blivious ping. Please let us know if you have a problem with the issue.

[0blivious]

@0blivious ping. Please let us know if you have a problem with the issue.

Sorry was a bit busy for these few days. I have updated the implementation to skip * in front. I have some problems with the CI though. Can tell me how to interpret the pitest result and fix it?

[rnveach]

Can tell me how to interpret the pitest result and fix it?

Please see #7797 on an introduction to pitest and some helpful links.

[romani]

@0blivious , please reply "done" to each my point to make sure nothing is missed.

[romani]

please regenerate report , there should be no violations - https://0blivious.github.io/sevntu-checkstyle/index.html

https://0blivious.github.io/Orekit/index.html#A36 valid violation
#A36 | warning | JavadocMissingWhitespaceAfterAsterisk | Missing a whitespace after the leading asterisk. | 52 | 8
, but .... take a look at source file
why there is no violation at line 55:

52      /**Index of the latest closest neighbor.*/
53      private int latestClosestNeighbor;
54  
55      /**Simple constructor.
56       * @param interpolationPoints number of points used in the interpolation
57       */
58      public ShortPeriodicsInterpolatedCoefficient(final int interpolationPoints) {

item to improve:

[0blivious]

please regenerate report , there should be no violations - https://0blivious.github.io/sevntu-checkstyle/index.html

https://0blivious.github.io/Orekit/index.html#A36 valid violation
#A36 | warning | JavadocMissingWhitespaceAfterAsterisk | Missing a whitespace after the leading asterisk. | 52 | 8
, but .... take a look at source file
why there is no violation at line 55:

I have regenerated the report and it report violation at line 55 correctly. However, the sevntu-checkstyle still report the same violation. It seems that it still test the old code. I am not sure why this happen......

[rnveach]

@0blivious If you are referring to the diff script regression, it does not update the repositories automatically. You will have to do this manually.
checkstyle/contribution#180

[romani]

@0blivious , please remove all git clones from chekcstyle-tester to let him get latest sources.

[0blivious]

@romani Hi I have updated the report and fix the input file. Can I get your review on this?

[romani]

as it is change of code, please regenerate report, please confirm that these is not diff from previous implementation in violations.

last minor:

[romani]

@0blivious , please rebase on latest master to resolve wercker issue.

[romani]

as we change code, please do not forget to regenerate regression diff report.

new item:

[romani]

unfortunately I still do not like design .....
name hasWhitespaceAfterAsteriskBeforeText is not same as logic inside, it will be a problems in future.

please do:

if (textNode != null
  && textNode.getType() != JavadocTokenTypes.EOF) {
    int lastAsterisPosition = getLastLeadingAsterik(....)
    if (!isLast(lastAsterisPosition) && !isWhitespace(lastAsterisPosition +1  )) {
      log(textNode.getLineNumber(), textNode.getColumnNumber(), MSG_KEY);
    }
}

and please add tests case with tabs after leading asterik.
Please create new Input file for this as tabs are forbidden in java files in our repo, so we need to suppress violation on it.

[romani]

Please create new Input file for this as tabs are forbidden in java files in our repo, so we need to suppress violation on it.

sad leftover #8019, we do not validate such code for bad whitespaces.