Skip to content

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

Merged
jsotuyod merged 8 commits into
pmd:masterfrom
adangel:issue-1174
Jun 24, 2018
Merged

[java] Deprecated CommentUtil, move implementation to AST Comment#1198
jsotuyod merged 8 commits into
pmd:masterfrom
adangel:issue-1174

Conversation

@adangel

@adangel adangel commented Jun 21, 2018

Copy link
Copy Markdown
Member

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

adangel commented Jun 22, 2018

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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