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 1 commit
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
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,75 @@ 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 as a whole.
TWiStErRob marked this conversation as resolved.
Show resolved Hide resolved
* * single line comments are always separate, conjoint lines are not merged.
TWiStErRob marked this conversation as resolved.
Show resolved Hide resolved
* * 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 comment markers (`//`, `/​*`, `/​**`, `*` aligners, `*​/`) are removed before applying the regex.
* One leading space is removed from each line of the comment, after starting markers and aligners.
atulgpt marked this conversation as resolved.
Show resolved Hide resolved
* * 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.
*
* 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: 'some fixes are pending.'
* value: 'FIXME:'
* - reason: 'some changes are present which needs to be addressed before ship.'
* value: 'STOPSHIP:'
* - reason: 'some changes are pending.'
* 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'
* # Explanation: at the beginning of the line, optional leading space,
* # case insensitive (e.g. REVIEW, review, Review), and REVIEW only as a full word.
* # 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*$'
* # Explanation: full line with optional leading and trailing space, and an at @ character followed by a word,
* # but not @suppress or @hide because those don't need content afterwards.
* # Non-compliant: /​** ... @see *​/
*
* - 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 @@ -50,7 +110,7 @@ class ForbiddenComment(config: Config = Config.empty) : Rule(config) {
"TODO:" to "some changes are pending.",
)
) { list ->
list.map { Comment(it.value.toRegex(), it.reason) }
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`.")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,111 @@
}
}

@Nested
inner class `regex semantics for comments` {

@Test
fun `should report a finding at the beginning of lines`() {
val comment = """
/*
* foo bar
* baz qux
*/
""".trimIndent()
assertThat(ForbiddenComment(TestConfig(COMMENTS to listOf("^baz"))).compileAndLint(comment)).hasSize(1)
}

@Test
fun `should not report a finding at the beginning of lines when the flag is off`() {
val comment = """
/*
* foo bar
* baz qux
*/
""".trimIndent()
assertThat(ForbiddenComment(TestConfig(COMMENTS to listOf("(?-m)^baz"))).compileAndLint(comment)).isEmpty()
}

@Test
fun `should report a finding at the end of lines`() {
val comment = """
/*
* foo bar
* baz qux
*/
""".trimIndent()
assertThat(ForbiddenComment(TestConfig(COMMENTS to listOf("bar$"))).compileAndLint(comment)).hasSize(1)
}

@Test
fun `should not report a finding at the end of lines when flag is off`() {
val comment = """
/*
* foo bar
* baz qux
*/
""".trimIndent()
assertThat(ForbiddenComment(TestConfig(COMMENTS to listOf("(?-m)bar$"))).compileAndLint(comment)).isEmpty()
}

@Test
fun `should report a finding at the beginning of a comment`() {
val comment = """
/*
* foo bar
* baz qux
*/
""".trimIndent()
assertThat(ForbiddenComment(TestConfig(COMMENTS to listOf("^foo"))).compileAndLint(comment)).hasSize(1)
assertThat(ForbiddenComment(TestConfig(COMMENTS to listOf("\\Afoo"))).compileAndLint(comment)).hasSize(1)
}

@Test
fun `should report a finding at the end of a comment`() {
val comment = """
/*
* foo bar
* baz qux
*/
""".trimIndent()
assertThat(ForbiddenComment(TestConfig(COMMENTS to listOf("qux$"))).compileAndLint(comment)).hasSize(1)
assertThat(ForbiddenComment(TestConfig(COMMENTS to listOf("qux\\Z"))).compileAndLint(comment)).hasSize(1)
}

@Test
fun `should report a finding across lines`() {
val comment = """
/*
* foo bar
* baz qux
*/
""".trimIndent()
assertThat(ForbiddenComment(TestConfig(COMMENTS to listOf("^foo.*qux$"))).compileAndLint(comment)).hasSize(1)
Fixed Show fixed Hide fixed
Fixed Show fixed Hide fixed
}

@Test
fun `should not report a finding across lines when the flag is off`() {
val comment = """
/*
* foo bar
* baz qux
*/
""".trimIndent()
assertThat(ForbiddenComment(TestConfig(COMMENTS to listOf("(?-s)^foo.*qux$"))).compileAndLint(comment)).isEmpty()
}

@Test
fun `should report all separate findings at once`() {
val comment = """
/*
* foo baz
* bar qux
*/
""".trimIndent()
assertThat(ForbiddenComment(TestConfig(COMMENTS to listOf("^foo", "^bar"))).compileAndLint(comment)).hasSize(2)
Fixed Show fixed Hide fixed
Fixed Show fixed Hide fixed
}
TWiStErRob marked this conversation as resolved.
Show resolved Hide resolved
}

@TestInstance(TestInstance.Lifecycle.PER_CLASS)
@Nested
inner class `comment getContent` {
Expand Down