[java] Cleanup handling of Java comments #4237
Conversation
Generated by 🚫 Danger |
adangel
added
the
in:grammar
| * Wraps a comment token to provide some utilities. | ||
| * This is not a node, it's not part of the tree anywhere, | ||
| * just convenient. |
There was a problem hiding this comment.
Do you think, this would make sense at some point in the future? To add the comments as nodes to the tree?
| @SuppressWarnings("PMD.LiteralsFirstInComparisons") // a fp | ||
| public static boolean isMarkupWord(Chars word) { | ||
| return word.length() <= 3 | ||
| && (word.contentEquals("*") | ||
| || word.contentEquals("//") | ||
| || word.contentEquals("/*") | ||
| || word.contentEquals("*/") | ||
| || word.contentEquals("/**")); | ||
| } |
There was a problem hiding this comment.
I want to understand, why this is a FP...
is it, because you would get a NP anyway with word.length(), so putting the literals first in contentEquals doesn't make sense here? (or in other words: word can't be null when calling contentEquals)
There was a problem hiding this comment.
I don't know if that's relevant anymore, I wrote this code before we updated the rule in pmd 7. The problem at the time was that the rule saw a method name that ends with equals (contentEquals) and reported the method call. However the literal and the word have different types, so swapping the operands would call String::contentEquals, when the method I want to call is Chars::contentEquals.
| NON_REGEX_PROPERTIES.add(CASE_SENSITIVE_DESCRIPTOR); | ||
| } | ||
| private static final PropertyDescriptor<Pattern> DISSALLOWED_TERMS_DESCRIPTOR = | ||
| regexProperty("forbiddenRegex") |
There was a problem hiding this comment.
We should add a note in 7_0_0_release_notes.md under "Changed Rules": For "CommentContent" the properties "caseSensitive" and "disallowedTerms" are replaced by the regex property "fobiddenRegex".
[java] Cleanup handling of Java comments #4237
| int i = 0; | ||
| for (Chars line : comment.getFilteredLines(true)) { | ||
| if (violationRegex.matcher(line).find()) { | ||
| lines.add(i); |
There was a problem hiding this comment.
looks like this doesn't work, i is never incremented.
There was a problem hiding this comment.
| private boolean shouldReportTypeDeclaration(ASTAnyTypeDeclaration decl) { | ||
| // don't report on interfaces | ||
| return !decl.isRegularInterface() | ||
| return !(decl.isRegularInterface() && !decl.isAnnotation()) |
There was a problem hiding this comment.
this could be replaced with !decl.isInterface(). I don't think it makes sense to whitelist interfaces though. If they have package visibility then they should be reported. Maybe this was originally an attempt to whitelist interface members, because they're are all public anyway...
| @@ -63,10 +63,10 @@ public Sequence call(XPathContext context, Sequence[] arguments) { | |||
| int codeBeginLine = contextNode.getBeginLine(); | |||
| int codeEndLine = contextNode.getEndLine(); | |||
|
|
|||
| List<Comment> commentList = contextNode.getFirstParentOfType(ASTCompilationUnit.class).getComments(); | |||
| for (Comment comment : commentList) { | |||
| List<JavaComment> commentList = contextNode.getFirstParentOfType(ASTCompilationUnit.class).getComments(); | |||
| int i = 0; | ||
| for (Chars line : comment.getFilteredLines(true)) { | ||
| if (line.length() > maxLength) { | ||
| indices.add(i); |
Describe the PR
Related issues
Ready?
./mvnw clean verifypasses (checked automatically by github actions)