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

[java] Deprecated CommentUtil, move implementation to AST Comment #1198

Merged
merged 8 commits into from
Jun 24, 2018

Conversation

adangel
Copy link
Member

@adangel adangel commented Jun 21, 2018

Fixes #1174

Work in progress - this is missing:

  • Mark the methods in CommentUtil as deprecated
  • Move the methods maybe into the Comment AST nodes
  • Release Notes (API changes)

@adangel adangel added a:bug PMD crashes or fails to analyse a file. is:WIP For PRs that are not fully ready, or issues that are actively being tackled labels Jun 21, 2018
@adangel adangel added this to the 6.5.0 milestone Jun 21, 2018
@adangel adangel changed the title [java] Add Unit test for CommentUtil [java] Deprecated CommentUtil, move implementation to AST Comment Jun 22, 2018
@adangel
Copy link
Member Author

adangel commented Jun 22, 2018

That was a interesting rabbit whole 😄
Most methods of CommentUtil didn't work (at least, not as I expected). The code has also been duplicated into AbstractCommentRule (e.g.

- looks familiar - this even had the bug already fixed...)

So interesting is: FormalComment has basic support for javadoc tags: In Comment we parse the javadoc and add JavadocElement nodes as children... I moved the code into FormalComment.
It's of course far away from being complete, but could be a interesting way of exposing the javadoc information.

@adangel adangel removed the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Jun 22, 2018
@oowekyala oowekyala mentioned this pull request Jun 22, 2018
64 tasks
* @return List of lines of the comments
*/
private List<String> multiLinesIn() {
String[] lines = getImage().split("\\R");
Copy link
Member

Choose a reason for hiding this comment

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

support for the line break matcher is Java8+ only. We can however use the extended version of this:

\u000D\u000A|[\u000A\u000B\u000C\u000D\u0085\u2028\u2029]

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, didn't know that...

boolean foundFirstNonEmptyLine = false;
for (String line : lines) {
if (StringUtils.isNoneBlank(line)) {
// new non-empty line: add all previous empty lines occurred before
Copy link
Member

Choose a reason for hiding this comment

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

isNoneBlank doesn't check the line is not empty, but actually if not a single character is a whitespace. So, "this is my comment" will fail this check for having 3 whitespaces. Is this really what you expect?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess, I wanted to use isNotBlank - isNoneBlank is the same, but for multiple lines (it accepts a vararg ... String). So, the plural here would refer to multiple lines, not to multiple characters...

private void findJavadocs(String commentText) {
Collection<JavadocElement> kids = new ArrayList<>();

Map<String, Integer> tags = CommentUtil.javadocTagsIn(commentText);
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we avoid internal usage of CommentUtil as we are deprecating it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've missed that...

@jsotuyod jsotuyod merged commit 913fe67 into pmd:master Jun 24, 2018
@adangel adangel deleted the issue-1174 branch July 8, 2018 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:bug PMD crashes or fails to analyse a file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants