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

Add comments with a list of regexes to ForbiddenComment #5981

Merged
merged 30 commits into from
May 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
6400988
Add valuePatterns list of regexes
atulgpt Apr 10, 2023
5eba48b
Apply suggestions from code review
atulgpt Apr 11, 2023
6ef834c
Deprecate values option
atulgpt Apr 11, 2023
06350d1
Check against complete text
atulgpt Apr 13, 2023
d9dc02f
Apply suggestions from code review
atulgpt May 6, 2023
6e22238
Update the `valuePatterns` config name to `comments`
atulgpt May 6, 2023
e2a3af0
Use static import of `io.gitlab.arturbosch.detekt.test.assertThat`
atulgpt May 6, 2023
7580f5c
Use `valuesWithReason` in `comments` config
atulgpt May 15, 2023
8e11b39
Incorporate review comments and align comments for indented code
atulgpt May 16, 2023
42a80c7
Update detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt…
atulgpt May 18, 2023
373bc23
Refactor code make `trimIndentIgnoringFirstLine` out
atulgpt May 18, 2023
a97dcab
Shorten name: if it's trimmed, it cannot be also "full"
TWiStErRob May 19, 2023
b747f77
Test name fixes
TWiStErRob May 19, 2023
007d3b4
Add tests for trailing and other mixed code-comment pairs
TWiStErRob May 19, 2023
4687325
Migrate detekt config to use new property. Fixes:
TWiStErRob May 19, 2023
ffff4f7
Format for visibility
TWiStErRob May 19, 2023
b4ca396
Document behavior, add tests for documented behaviors and fix them.
TWiStErRob May 19, 2023
5673f92
De-noise ForbiddenComment instance creations
TWiStErRob May 19, 2023
60d53f1
Fix test snippets
TWiStErRob May 19, 2023
3c31fe0
Update detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt…
atulgpt May 19, 2023
ee18e1a
Apply suggestions from code review
atulgpt May 19, 2023
b6f35e9
Refactor `report` to common `reportCodeSmell` method
atulgpt May 19, 2023
a5be5a4
Update TC to reflect the updated message
atulgpt May 19, 2023
1565fb6
Update the detekt.yml `ForbiddenComment` `reason` config to match the…
atulgpt May 19, 2023
f4c59c1
Improve marker removal docs
TWiStErRob May 19, 2023
948cb73
Clarify why it matters.
TWiStErRob May 19, 2023
0a1763f
Update the warning message to only use `reason` if available
atulgpt May 19, 2023
b03bd56
Remove not necessary ZWSP
atulgpt May 19, 2023
fe491f6
Update message to remove ` in detekt`
atulgpt May 19, 2023
6166297
Minor clarifications and formatting.
TWiStErRob May 20, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 11 additions & 6 deletions config/detekt/detekt.yml
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,17 @@ style:
active: true
ForbiddenComment:
active: true
values:
- 'TODO:'
- 'FIXME:'
- 'STOPSHIP:'
- '@author'
- '@requiresTypeResolution'
comments:
- value: 'FIXME:'
reason: 'Forbidden FIXME todo marker in comment, please fix the problem.'
- value: 'STOPSHIP:'
reason: 'Forbidden STOPSHIP todo marker in comment, please address the problem before shipping the code.'
- value: 'TODO:'
reason: 'Forbidden TODO todo marker in comment, please do the changes.'
- value: '@author'
reason: 'Authors are not recorded in KDoc.'
- value: '@requiresTypeResolution'
reason: 'Use @RequiresTypeResolution annotation on the class instead.'
excludes: ['**/detekt-rules-style/**/ForbiddenComment.kt']
ForbiddenImport:
active: true
Expand Down
12 changes: 7 additions & 5 deletions detekt-core/src/main/resources/default-detekt-config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -566,12 +566,14 @@ style:
value: 'java.lang.annotation.Inherited'
ForbiddenComment:
active: true
values:
- 'FIXME:'
- 'STOPSHIP:'
- 'TODO:'
TWiStErRob marked this conversation as resolved.
Show resolved Hide resolved
comments:
- reason: 'Forbidden FIXME todo marker in comment, please fix the problem.'
value: 'FIXME:'
- reason: 'Forbidden STOPSHIP todo marker in comment, please address the problem before shipping the code.'
value: 'STOPSHIP:'
- reason: 'Forbidden TODO todo marker in comment, please do the changes.'
value: 'TODO:'
allowedPatterns: ''
customMessage: ''
ForbiddenImport:
active: false
imports: []
Expand Down
2 changes: 2 additions & 0 deletions detekt-core/src/main/resources/deprecation.properties
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ potential-bugs>IgnoredReturnValue>restrictToAnnotatedMethods=Use `restrictToConf
potential-bugs>LateinitUsage>excludeAnnotatedProperties=Use `ignoreAnnotated` instead
potential-bugs>MissingWhenCase=Rule deprecated as compiler performs this check by default
potential-bugs>RedundantElseInWhen=Rule deprecated as compiler performs this check by default
style>ForbiddenComment>customMessage=Use `comments` and provide `reason` against each `value`.
style>ForbiddenComment>values=Use `comments` instead, make sure you escape your text for Regular Expressions.
style>ForbiddenPublicDataClass=Rule migrated to `libraries` ruleset plugin
style>FunctionOnlyReturningConstant>excludeAnnotatedFunction=Use `ignoreAnnotated` instead
style>LibraryCodeMustSpecifyReturnType=Rule migrated to `libraries` ruleset plugin
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,21 +10,80 @@ import io.gitlab.arturbosch.detekt.api.Severity
import io.gitlab.arturbosch.detekt.api.config
import io.gitlab.arturbosch.detekt.api.internal.ActiveByDefault
import io.gitlab.arturbosch.detekt.api.internal.Configuration
import io.gitlab.arturbosch.detekt.api.valuesWithReason
import org.jetbrains.kotlin.com.intellij.psi.PsiComment
import org.jetbrains.kotlin.com.intellij.psi.PsiElement
import org.jetbrains.kotlin.kdoc.psi.impl.KDocSection
import org.jetbrains.kotlin.psi.KtFile
import org.jetbrains.kotlin.psi.psiUtil.collectDescendantsOfType

// Note: ​ (zero-width-space) is used to prevent the Kotlin parser getting confused by talking about comments in a comment.
/**
* This rule allows to set a list of comments which are forbidden in the codebase and should only be used during
* development. Offending code comments will then be reported.
*
* The regular expressions in `comments` list will have the following behaviors while matching the comments:
* * **Each comment will be handled individually.**
* * single line comments are always separate, consecutive lines are not merged.
* * multi line comments are not split up, the regex will be applied to the whole comment.
* * KDoc comments are not split up, the regex will be applied to the whole comment.
* * **The following comment delimiters (and indentation before them) are removed** before applying the regex:
* `//`, `// `, `/​*`, `/​* `, `/​**`, `*` aligners, `*​/`, ` *​/`
* * **The regex is applied as a multiline regex**,
* see [Anchors](https://www.regular-expressions.info/anchors.html) for more info.
* To match the start and end of each line, use `^` and `$`.
TWiStErRob marked this conversation as resolved.
Show resolved Hide resolved
* To match the start and end of the whole comment, use `\A` and `\Z`.
* To turn off multiline, use `(?-m)` at the start of your regex.
* * **The regex is applied with dotall semantics**, meaning `.` will match any character including newlines,
* this is to ensure that freeform line-wrapping doesn't mess with simple regexes.
* To turn off this behavior, use `(?-s)` at the start of your regex, or use `[^\r\n]*` instead of `.*`.
* * **The regex will be searched using "contains" semantics** not "matches",
* so partial comment matches will flag forbidden comments.
* In practice this means there's no need to start and end the regex with `.*`.
*
* The rule can be configured to add extra comments to the list of forbidden comments, here are some examples:
* ```yaml
* ForbiddenComment:
* comments:
* # Repeat the default configuration if it's still needed.
* - reason: 'Forbidden FIXME todo marker in comment, please fix the problem.'
* value: 'FIXME:'
* - reason: 'Forbidden STOPSHIP todo marker in comment, please address the problem before shipping the code.'
* value: 'STOPSHIP:'
* - reason: 'Forbidden TODO todo marker in comment, please do the changes.'
* value: 'TODO:'
* # Add additional patterns to the list.
*
* - reason: 'Authors are not recorded in KDoc.'
* value: '@author'
*
* - reason: 'REVIEW markers are not allowed in production code, only use before PR is merged.'
* value: '^\s*(?i)REVIEW\b'
* # Non-compliant: // REVIEW this code before merging.
* # Compliant: // Preview will show up here.
*
* - reason: 'Use @androidx.annotation.VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) instead.'
* value: '^private$'
* # Non-compliant: /*private*/ fun f() { }
*
* - reason: 'KDoc tag should have a value.'
* value: '^\s*@(?!suppress|hide)\w+\s*$'
* # Non-compliant: /** ... @see */
* # Compliant: /** ... @throws IOException when there's a network problem */
*
* - reason: 'include an issue link at the beginning preceded by a space'
* value: 'BUG:(?! https://github\.com/company/repo/issues/\d+).*'
* ```
*
* By default the commonly used todo markers are forbidden: `TODO:`, `FIXME:` and `STOPSHIP:`.
*
* <noncompliant>
* val a = "" // TODO: remove please
* // FIXME: this is a hack
* /**
* * FIXME: this is a hack
* */
* fun foo() { }
* // STOPSHIP:
* /* STOPSHIP: */
cortinico marked this conversation as resolved.
Show resolved Hide resolved
* </noncompliant>
*/
@ActiveByDefault(since = "1.0.0")
Expand All @@ -38,17 +97,31 @@ class ForbiddenComment(config: Config = Config.empty) : Rule(config) {
)

@Configuration("forbidden comment strings")
private val values: List<String> by config(listOf("FIXME:", "STOPSHIP:", "TODO:"))
@Deprecated("Use `comments` instead, make sure you escape your text for Regular Expressions.")
private val values: List<String> by config(emptyList())

@Configuration("forbidden comment string patterns")
private val comments: List<Comment> by config(
valuesWithReason(
"FIXME:" to "Forbidden FIXME todo marker in comment, please fix the problem.",
"STOPSHIP:" to "Forbidden STOPSHIP todo marker in comment, " +
"please address the problem before shipping the code.",
"TODO:" to "Forbidden TODO todo marker in comment, please do the changes.",
)
) { list ->
list.map { Comment(it.value.toRegex(setOf(RegexOption.DOT_MATCHES_ALL, RegexOption.MULTILINE)), it.reason) }
}
TWiStErRob marked this conversation as resolved.
Show resolved Hide resolved

@Configuration("ignores comments which match the specified regular expression. For example `Ticket|Task`.")
private val allowedPatterns: Regex by config("", String::toRegex)

@Configuration("error message which overrides the default one")
@Deprecated("Use `comments` and provide `reason` against each `value`.")
private val customMessage: String by config("")

override fun visitComment(comment: PsiComment) {
super.visitComment(comment)
val text = comment.text
val text = comment.getContent()
checkForbiddenComment(text, comment)
}

Expand All @@ -63,24 +136,87 @@ class ForbiddenComment(config: Config = Config.empty) : Rule(config) {
private fun checkForbiddenComment(text: String, comment: PsiElement) {
if (allowedPatterns.pattern.isNotEmpty() && allowedPatterns.containsMatchIn(text)) return

@Suppress("DEPRECATION")
values.forEach {
if (text.contains(it, ignoreCase = true)) {
report(
CodeSmell(
issue,
Entity.from(comment),
getErrorMessage(it)
)
)
reportIssue(comment, getErrorMessage(it))
}
}

comments.forEach {
if (it.value.containsMatchIn(text)) {
reportIssue(comment, getErrorMessage(it))
}
}
}

private fun reportIssue(comment: PsiElement, msg: String) {
report(
CodeSmell(
issue,
Entity.from(comment),
msg
)
)
}

private fun PsiComment.getContent(): String = text.getCommentContent()

private fun getErrorMessage(comment: Comment): String =
comment.reason ?: String.format(DEFAULT_ERROR_MESSAGE, comment.value.pattern)

@Suppress("DEPRECATION")
private fun getErrorMessage(value: String): String =
customMessage.takeUnless { it.isEmpty() } ?: String.format(DEFAULT_ERROR_MESSAGE, value)
customMessage.takeUnless { it.isEmpty() }
?: String.format(DEFAULT_ERROR_MESSAGE, value)

private data class Comment(val value: Regex, val reason: String?)

companion object {
const val DEFAULT_ERROR_MESSAGE = "This comment contains '%s' " +
"that has been defined as forbidden in detekt."
const val DEFAULT_ERROR_MESSAGE = "This comment contains '%s' that has been defined as forbidden."
}
}

internal fun String.getCommentContent(): String {
TWiStErRob marked this conversation as resolved.
Show resolved Hide resolved
atulgpt marked this conversation as resolved.
Show resolved Hide resolved
return if (this.startsWith("//")) {
this.removePrefix("//").removePrefix(" ")
} else {
this
.trimIndentIgnoringFirstLine()
// Process line by line.
.lineSequence()
// Remove starting, aligning and ending markers.
.map {
it
.let { fullLine ->
val trimmedStartLine = fullLine.trimStart()
if (trimmedStartLine.startsWith("/*")) {
trimmedStartLine.removePrefix("/*").removePrefix(" ")
} else if (trimmedStartLine.startsWith("*") && trimmedStartLine.startsWith("*/").not()) {
trimmedStartLine.removePrefix("*").removePrefix(" ")
} else {
fullLine
}
}
.let { lineWithoutStartMarker ->
if (lineWithoutStartMarker.endsWith("*/")) {
lineWithoutStartMarker.removeSuffix("*/").removeSuffix(" ")
} else {
lineWithoutStartMarker
}
}
}
// Trim trailing empty lines.
.dropWhile(String::isEmpty)
// Reconstruct the comment contents.
.joinToString("\n")
}
}

private fun String.trimIndentIgnoringFirstLine(): String =
if ('\n' !in this) {
this
} else {
val lines = this.lineSequence()
lines.first() + "\n" + lines.drop(1).joinToString("\n").trimIndent()
}