Issue #4717: Added JavadocParserErrorStrategy which extends BailErrorStrategy by therootusr · Pull Request #4825 · checkstyle/checkstyle

therootusr · 2017-07-25T22:08:15Z

therootusr · 2017-07-25T22:11:23Z

@Vladlis Thank you for the solution. Please have a look.

codecov-io · 2017-07-25T23:04:05Z

Codecov Report

Merging #4825 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #4825   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         288     288           
  Lines       15435   15435           
  Branches     3501    3500    -1     
======================================
  Hits        15435   15435

Impacted Files	Coverage Δ
...heckstyle/checks/javadoc/AbstractJavadocCheck.java	`100% <ø> (ø)`	⬆️
...rawl/tools/checkstyle/JavadocDetailNodeParser.java	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46fa809...57d1070. Read the comment docs.

rnveach · 2017-07-26T00:09:29Z

@PS-SP TC failed.

therootusr · 2017-07-26T00:26:12Z

@rnveach Yes, I checked. One reason is throwing unchecked exception in code which can be taken care of but another is with the unused catch parameter here at JavadocDetailNodeParser.java#L129. Right now we don't need to do anything with that exception object.

therootusr · 2017-07-26T00:37:07Z

diff report

Vladlis · 2017-07-26T07:51:27Z

@PS-SP
PR and report look good.
Please either resolve or suppress (with explanation) TC violations.

therootusr · 2017-07-26T08:25:16Z

@Vladlis

Please either resolve or suppress (with explanation) TC violations.

Resolved.

rnveach · 2017-07-26T10:51:36Z

src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/AbstractJavadocCheckTest.java

 import static com.puppycrawl.tools.checkstyle.checks.javadoc.AbstractJavadocCheck.MSG_JAVADOC_MISSED_HTML_CLOSE;
 import static com.puppycrawl.tools.checkstyle.checks.javadoc.AbstractJavadocCheck.MSG_JAVADOC_PARSE_RULE_ERROR;
 import static com.puppycrawl.tools.checkstyle.checks.javadoc.AbstractJavadocCheck.MSG_JAVADOC_WRONG_SINGLETON_TAG;
-import static com.puppycrawl.tools.checkstyle.checks.javadoc.AbstractJavadocCheck.MSG_KEY_UNRECOGNIZED_ANTLR_ERROR;


Is this message no longer possible to receive? If so, shouldn't we remove the field and properties message?

Yes it shouldn't be possible any longer. This one at messages.properties#L1 also seems unutilized.

The reason I didn't remove unrecognized one is that right now it looks like that report should be called under all circumstances. But it might so happen that under some weird input or for some other reason ANTLR fails to report to listener and then we might again require it. So I thought that after some time of usage when we can be sure that never does ANTLR fail to report to listener, we can remove all the unutilized message keys together in one issue so that all other message.properties get cleaned.

I still think it should be removed. We will still have it's code in history if we need it again.
@romani You agree?

@PS-SP
Please remove this property.
@rnveach @romani
I thought we have some inspection to detect unused properties. If not - should we add one?

@Vladlis It is a public property, so it probably can't tell the difference between something we give to the users and something we just use for ourselves. Had it been private, it probably would have thrown up a flag.

@Vladlis

Please remove this property.

Done

rnveach · 2017-07-26T18:03:01Z

http://4717-diff.getforge.io/apache-ant/index.html#A26
http://4717-diff.getforge.io/apache-ant/xref/home/cortana/Desktop/checkstyle/contribution/checkstyle-tester/repositories/apache-ant/src/main/org/apache/tools/zip/Simple8BitZipEncoding.java.html#L234

Details: mismatched input 'org.apache.tools.zip' expecting <EOF> while parsing JAVADOC
@see org.apache.tools.zip.ZipEncoding#encode(java.lang.String)

Why does this fail recognition?
According to http://www.oracle.com/technetwork/articles/java/index-137868.html , it should be acceptable: @see package.Class#method(Type, Type,...)

Edit: It seems the line separation between @see and org is causing the failure. This should be an issue as it looks valid to me.

therootusr · 2017-07-26T18:19:16Z

@rnveach
Because of the LEADING_ASTERISK, which is supported in DESCRIPTION in here at JavadocParser.g4#L879. reference rule doesn't support LEADING_ASTERISK

rnveach · 2017-07-26T18:53:38Z

@PS-SP If you agree it should be acceptable, make a issue for it. Otherwise explain why it isn't valid javadoc.

therootusr · 2017-07-26T18:54:31Z

@rnveach I agree it should be valid. @see tag is already being worked under #4752. We can change that issue topic if need be

rnveach · 2017-07-26T19:50:01Z

@PS-SP Since you already started that issue, unless your fix already includes the fix for the issue I pointed at, I think it should be a new issue. Placing too much into 1 issue over complicates the issue and will be harder to manage.

therootusr · 2017-07-26T22:52:28Z

@rnveach Can be done there itself. Please check my summary comment there I just made.

rnveach · 2017-07-26T23:34:59Z

@PS-SP And your long winded post proves to me that too much is going on there.
Please rebase to resolve conflicts.

therootusr · 2017-07-26T23:59:18Z

@rnveach Rebased.

That PR wasn't intended to drag on for so long and to be so big. But things kept coming one after the other and here we are. Just one little problem needs to be addressed now and hopefully I will be able to update PR finally.

rnveach · 2017-07-27T00:26:47Z

@romani Please review.

romani · 2017-07-27T13:09:03Z

@PS-SP , please rebase.
changes in property files are unfortunate , but all was done in them is a sorting. I hope it will not make too much problems to rebase.

I like more detailed messaged. As CI pass, we good to merge this PR.

… BailErrorStrategy

therootusr · 2017-07-27T20:36:04Z

@romani rebased

therootusr force-pushed the issue_4717 branch from 08c5af3 to 578f48f Compare July 26, 2017 08:22

Vladlis approved these changes Jul 26, 2017

View reviewed changes

Vladlis requested review from rnveach and romani July 26, 2017 09:27

rnveach reviewed Jul 26, 2017

View reviewed changes

therootusr force-pushed the issue_4717 branch from 578f48f to 61fded7 Compare July 26, 2017 18:59

therootusr mentioned this pull request Jul 26, 2017

Issue #4752: Enforced WS before javadoc tag @see. Added semantic predicate for PACKAGE token in ANTLR grammar for javadoc #4753

Closed

therootusr force-pushed the issue_4717 branch from 61fded7 to d9a68c8 Compare July 26, 2017 23:55

rnveach approved these changes Jul 27, 2017

View reviewed changes

romani approved these changes Jul 27, 2017

View reviewed changes

Issue checkstyle#4717: Added JavadocParserErrorStrategy which extends…

57d1070

… BailErrorStrategy

therootusr force-pushed the issue_4717 branch from d9a68c8 to 57d1070 Compare July 27, 2017 20:34

Vladlis merged commit ebeedf9 into checkstyle:master Jul 28, 2017

Uh oh!

Conversation

therootusr commented Jul 25, 2017

Uh oh!

therootusr commented Jul 25, 2017

Uh oh!

codecov-io commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rnveach commented Jul 26, 2017

Uh oh!

therootusr commented Jul 26, 2017

Uh oh!

therootusr commented Jul 26, 2017

Uh oh!

Vladlis commented Jul 26, 2017

Uh oh!

therootusr commented Jul 26, 2017

Uh oh!

rnveach Jul 26, 2017

Choose a reason for hiding this comment

Uh oh!

therootusr Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnveach Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vladlis Jul 26, 2017

Choose a reason for hiding this comment

Uh oh!

rnveach Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

therootusr Jul 26, 2017

Choose a reason for hiding this comment

Uh oh!

rnveach commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

therootusr commented Jul 26, 2017

Uh oh!

rnveach commented Jul 26, 2017

Uh oh!

therootusr commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnveach commented Jul 26, 2017

Uh oh!

therootusr commented Jul 26, 2017

Uh oh!

rnveach commented Jul 26, 2017

Uh oh!

therootusr commented Jul 26, 2017

Uh oh!

rnveach commented Jul 27, 2017

Uh oh!

romani commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

therootusr commented Jul 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Jul 25, 2017 •

edited

Loading

therootusr Jul 26, 2017 •

edited

Loading

rnveach Jul 26, 2017 •

edited

Loading

rnveach Jul 26, 2017 •

edited

Loading

rnveach commented Jul 26, 2017 •

edited

Loading

therootusr commented Jul 26, 2017 •

edited

Loading

romani commented Jul 27, 2017 •

edited

Loading