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
Fixes false positive of trailing whitespaces in kdoc #6370
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #6370 +/- ##
=========================================
Coverage 85.18% 85.18%
- Complexity 4061 4067 +6
=========================================
Files 565 565
Lines 13321 13323 +2
Branches 2401 2401
=========================================
+ Hits 11347 11349 +2
Misses 773 773
Partials 1201 1201
☔ View full report in Codecov by Sentry. |
I'm not 100% sure about this one. Dokka doesn't support this. So they are really trailing spaces with no meaning. I would vote to close pr and issue until Dokka decides to support it. Otherwise I feel that we are adding false negatives more than adding a real feature. |
Hi, @BraisGabin I don't have a strong opinion on this issue. It's just that it is marked as a false positive in the bug tracker that's why I picked this issue. I am okay with any closing this issue as well |
I'm adding @TWiStErRob as reviewer here becuase it is his issue. I don't have a strong opinion here to be honest. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @BraisGabin, as I said in #5713 (comment).
I re-read the context, and while it's technically correct to support this, if KDoc doesn't do it, then people (like I did) will have a false sense that those spaces will do something.
Based on this I vote to close the PR; with the option to repurpose it in the future if/when feature is added, this is why I reviewed it. Until then, I think maybe opening a PR to update our docs and explain why those spaces are being flagged in "standard markdown" is the best way forward. That other PR could close the issue.
@@ -51,6 +52,15 @@ class TrailingWhitespace(config: Config = Config.empty) : Rule(config) { | |||
} | |||
} | |||
|
|||
private fun PsiElement.isKdocTrailingSpaces(trailingWhitespaces: Int): Boolean { | |||
return this.parent is KDocSection && trailingWhitespaces == 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Markdown spec says 2 or more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Than I think that the first two are correct but the others are trailing spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, makes sense to interpret it that way. If we go with "report an error as we do it now but we change the message", it can still be just >=2
because any number is unsupported right now.
@@ -51,6 +52,15 @@ class TrailingWhitespace(config: Config = Config.empty) : Rule(config) { | |||
} | |||
} | |||
|
|||
private fun PsiElement.isKdocTrailingSpaces(trailingWhitespaces: Int): Boolean { | |||
return this.parent is KDocSection && trailingWhitespaces == 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are nested list items still parented by KDocSection: nested list items?
return this.parent is KDocSection && trailingWhitespaces == 2 | ||
} | ||
|
||
private fun PsiElement.shouldReport(trailingWhitespacesCount: Int): Boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would flip this to simplify:
private fun PsiElement.shouldReport(trailingWhitespacesCount: Int): Boolean { | |
private fun PsiElement.isValid(trailingWhitespacesCount: Int): Boolean { |
fun `reports a excessive trailing spaces in kdoc`() { | ||
val code = """ | ||
/** | ||
* a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests look really funny without visible spaces, would
* a | |
* a$space$space$space |
be a stretch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also prevent future bulk clean-up of trailing spaces inside string literals.
fun `reports a excessive trailing spaces in kdoc`() { | ||
val code = """ | ||
/** | ||
* a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Align stars to mimic real(er) code.
And what if we report an error as we do it now but we change the message? We could say that this feature of markdown is not supported and link to the open issue. Even so, I don't see too much value here. To me we could directly close this PR and the issues directly. I would leave that decission to @atulgpt as he is the one doing the heavy lifting here. |
I like that! |
Hi @BraisGabin, @TWiStErRob I will update the PR incorporating your reviews |
@detekt/maintainers We need to take a decission here. A go or a not go to this PR. To me this is a not go and just close the issue as "work as expected". If Dokka fix the issue I would be more than happy to add this but now until that moment. |
Vote to close, but open another with a doc update to the rule (1 sentence) to explain what's happening. [or maybe detect the case, and provide a clear edge case error message, probably not worth the effort/maintenance] |
Same - good decision! |
Yup makes sense closing the issue.. o/w ROI was not justified |
Fixes #5713