SONARJAVA-5501 Implement check for syntax in Markdown comments S7474#5152
SONARJAVA-5501 Implement check for syntax in Markdown comments S7474#5152romainbrenguier merged 13 commits intomasterfrom
Conversation
romainbrenguier
force-pushed
the
romain/new-rule/markdown-javadoc-syntax
branch
5 times, most recently
from
7bf1828 to
f60597a
Compare
| @@ -0,0 +1,129 @@ | |||
| package checks; | |||
|
|
|||
| // Noncompliant@+3 {{replace HTML syntax with Markdown syntax in javadoc}} | |||
There was a problem hiding this comment.
It would be reallly nice to have secondary locations and quickfixes. If it seems doable, but too much work, then maybe a second PR would be an option?
There was a problem hiding this comment.
Secondary locations is probably easily doable but quickfixes probably much more difficult. I'll look at a solution for secondary locations first.
There was a problem hiding this comment.
Actually secondary locations cannot be done because the Location class requires a Tree syntax node, but we don't have that inside java comments.
|
|
||
| // Noncompliant@+2 | ||
| /// Here is a list: | ||
| /// <ul> |
There was a problem hiding this comment.
Is <ol> also in scope of this PR?
There was a problem hiding this comment.
I can add it
|
|
||
| @Override | ||
| public List<Tree.Kind> nodesToVisit() { | ||
| return Arrays.asList(PublicApiChecker.apiKinds()); |
There was a problem hiding this comment.
Starting iteration will have the effect of skipping dangling markdown docs, right? I think this is a good behavior and we should have a test for this, for example, the following code is compliant:
public void dangling() {
// Compliant, because it is not attached to a place where JavaDocs are legal.
/// Some text appears in <i>italic</i>.
}
There was a problem hiding this comment.
Yes, I'll add a test
| } | ||
| } | ||
|
|
||
| record Position(int lineNumber, int columnNumber) { |
There was a problem hiding this comment.
There is an existing class with the same name org.sonar.plugins.java.api.location.Position and it is 1-based. This one is 0-based right. It may get confusing in future.
| for (SyntaxTrivia trivia : markdownJavadoc) { | ||
| String comment = trivia.comment(); | ||
| Matcher matcher = NON_MARKDOWN_JAVADOC_PATTERN.matcher(comment); | ||
| for (Pair<Integer, Integer> range : rangeOfNonQuotedCode(comment)) { |
There was a problem hiding this comment.
There are lots of custom solutions for locations and ranges in text here. Have you considered using existing Position and Range classes? If they are not appropriate, have you considered extracting the utilities that you wrote into a separated reusable subclasses?
There was a problem hiding this comment.
I will rewrite that part using the existing LineColumnConverter utility.
romainbrenguier
force-pushed
the
romain/new-rule/markdown-javadoc-syntax
branch
3 times, most recently
from
b06eb77 to
07dcc14
Compare
| Matcher matcher = NON_MARKDOWN_JAVADOC_PATTERN.matcher(comment); | ||
| for (Pair<Integer, Integer> range : rangeOfNonQuotedCode(comment)) { | ||
| LineColumnConverter lineColumnConverter = new LineColumnConverter(comment); | ||
| List<Pair<Integer, Integer>> rangeOfNonQuotedCode = rangeOfNonQuotedCode(comment); |
There was a problem hiding this comment.
Quoted text complicates this check in a few places. Did you consider preprocessing comment by replacing quoted ranges with, say, (space), so that regex matching can be done without considering ranges?
There was a problem hiding this comment.
I thought about it, then thought ranges would be better. In the end, you are right, ranges are too complicated. Replacing with space makes this much simpler.
| return toPos(absolutSourcePosition).toPosition(); | ||
| } | ||
|
|
||
| public record Pos(int line, int columnOffset) { |
There was a problem hiding this comment.
Since we're here, let's put a JavaDoc here explaining what's 1-based and what's 0-based.
| } | ||
|
|
||
| /** | ||
| * Represent the position in a source String.The first character is at {@code line} 1, and {@code columnOffset} 0. |
There was a problem hiding this comment.
Space after full stop.
…d characters with blanks
romainbrenguier
force-pushed
the
romain/new-rule/markdown-javadoc-syntax
branch
from
c4d522c to
de6ed6f
Compare
|
2 participants
SONARJAVA-5501