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 #4934: Enforced WS in appropriate places for block javadoc tags #4938

Merged
merged 1 commit into from Aug 24, 2017

Conversation

voidfist
Copy link
Contributor

Issue #4934 : Enforced WS in appropriate places for block javadoc tags

@voidfist
Copy link
Contributor Author

diffreport-JavadocParagraph
ANTLR diff

The diffs are due to wrong javadoc which resulted in parse error and so the diffs should be acceptable

@codecov-io
Copy link

codecov-io commented Aug 11, 2017

Codecov Report

Merging #4938 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #4938   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         293     293           
  Lines       15889   15889           
  Branches     3603    3603           
======================================
  Hits        15889   15889

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 1fb6107...8496518. Read the comment docs.


| EXCEPTION_LITERAL (WS | NEWLINE)* CLASS_NAME? (WS | NEWLINE)* description?
| EXCEPTION_LITERAL (WS | NEWLINE)+ CLASS_NAME? (WS | NEWLINE)* ((WS | NEWLINE) description)?
Copy link
Contributor

Choose a reason for hiding this comment

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

(WS | NEWLINE)+ - will this work for /** @exception*/ ? Please add such inputs in UTs for every tag

Copy link
Contributor Author

@voidfist voidfist Aug 12, 2017

Choose a reason for hiding this comment

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

@Vladlis
Updated PR please have a look.

IMPORTANT: Such javadocs are wrong and should be corrected in #4942

@voidfist voidfist force-pushed the issue_4934 branch 3 times, most recently from 5dcd9d8 to 8136bc7 Compare August 12, 2017 19:09
@Vladlis Vladlis requested review from romani and rnveach August 13, 2017 11:57
@Vladlis
Copy link
Contributor

Vladlis commented Aug 16, 2017

@rnveach , @romani , Please review

@rnveach
Copy link
Member

rnveach commented Aug 17, 2017

I don't see any changes in code coverage in pom. Did the percentage stay the same and not increase?
Are we sure we covered all new grammar code?

@voidfist
Copy link
Contributor Author

@rnveach

Sorry I missed your comment. The coverage had decreased as the grammar was modified. Therefore the coverage was restored instead of getting increased. We should have covered "almost" all new grammar code as all that was done was changing the way (WS | NEWLINE) was to be treated and there were already tons of cases where javadoc tags had (WS | NEWLINE) before their arguments.

@romani
Copy link
Member

romani commented Aug 21, 2017

@PS-SP , please do rebase.

please add test case from issue. Issue test case always have to be in UT cases.

=============

Note:

(WS | NEWLINE)* ((WS | NEWLINE) description)?

should become (WS | NEWLINE)+ description in scope of #4942 , I approve the issue.

@voidfist voidfist force-pushed the issue_4934 branch 4 times, most recently from 4b260b4 to a8e62c7 Compare August 21, 2017 17:46
@voidfist
Copy link
Contributor Author

voidfist commented Aug 21, 2017

@romani
Rebased.

please add test case from issue

Done. Added one more UT. UT is in DetailNodeTreeStringPrinterTest.

@Vladlis
Copy link
Contributor

Vladlis commented Aug 21, 2017

@PS-SP please add such UTs for every affected tag

@voidfist
Copy link
Contributor Author

voidfist commented Aug 22, 2017

Appveyor failure relates to #4980 and is due to cases like the following in an input file:

* @exception
      Exception     Exception-description
      spanning multiple lines
* @param
      ParamName
      Param-description
* @throws
     Exception     Exception-description
         spanning multiple lines

Removing these cases decreases the coverage. So coverage must decrease a bit for this PR.

@voidfist
Copy link
Contributor Author

I had expected JavadocParser coverage to decrease a bit when upon removing the examples mentioned in the above cases. But apparently it didn't. Also, JavadocLexer coverage has increased which is fine.

Please correct me if wrong but I have not been able to find any package for which the coverage decreased in cobertura reports, still the total line and total branch coverage decreased by 1 %. May be some rounding off might be involved here and there.

Please have a look @Vladlis

@Vladlis
Copy link
Contributor

Vladlis commented Aug 22, 2017

@PS-SP , please enforce lf line ending for problematic cases in https://github.com/checkstyle/checkstyle/blob/master/.gitattributes until #4980

@voidfist voidfist force-pushed the issue_4934 branch 5 times, most recently from edd0947 to 8496518 Compare August 24, 2017 02:12
@voidfist
Copy link
Contributor Author

@Vladlis

please enforce lf line ending for problematic cases in https://github.com/checkstyle/checkstyle/blob/master/.gitattributes until #4980

Done.

@Vladlis
Copy link
Contributor

Vladlis commented Aug 24, 2017

@romani , all test cases are added, please review

@romani romani merged commit e51c3b6 into checkstyle:master Aug 24, 2017
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

5 participants