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.

Sign up for GitHub

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

Jump to bottom

Fix false positives in UndocumentedPublicProperty #2591

Merged
merged 3 commits into from Apr 11, 2020
Merged

Fix false positives in UndocumentedPublicProperty #2591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
11ea43a
Fix false positives in UndocumentedPublicProperty
schalkms Apr 7, 2020
4f7f8e8
Fix detekt warning
schalkms Apr 7, 2020
c1e44a6
Remove unnecessary test
schalkms Apr 7, 2020
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
14 changes: 14 additions & 0 deletions detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/Traversing.kt
View file
Open in desktop
Expand Up @@ -2,6 +2,9 @@ package io.gitlab.arturbosch.detekt.rules

import org.jetbrains.kotlin.com.intellij.psi.PsiElement
import org.jetbrains.kotlin.psi.KtElement
import org.jetbrains.kotlin.psi.KtNamedDeclaration
import org.jetbrains.kotlin.psi.psiUtil.containingClassOrObject
import org.jetbrains.kotlin.psi.psiUtil.isPublic

/**
* Returns a list of all parents of type [T] before first occurrence of [S].
Expand All @@ -16,3 +19,14 @@ inline fun <reified T : KtElement, reified S : KtElement> KtElement.parentsOfTyp
current = current.parent
}
}

internal fun KtNamedDeclaration.isPublicInherited(): Boolean {
var classOrObject = containingClassOrObject
while (classOrObject != null) {
if (!classOrObject.isPublic) {
return false
}
classOrObject = classOrObject.containingClassOrObject
}
return true
}
12 changes: 8 additions & 4 deletions ...main/kotlin/io/gitlab/arturbosch/detekt/rules/documentation/UndocumentedPublicProperty.kt
View file
Open in desktop
Expand Up @@ -7,12 +7,14 @@ import io.gitlab.arturbosch.detekt.api.Entity
import io.gitlab.arturbosch.detekt.api.Issue
import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.Severity
import io.gitlab.arturbosch.detekt.rules.isPublicInherited
import io.gitlab.arturbosch.detekt.rules.isPublicNotOverridden
import org.jetbrains.kotlin.psi.KtNamedDeclaration
import org.jetbrains.kotlin.psi.KtParameter
import org.jetbrains.kotlin.psi.KtPrimaryConstructor
import org.jetbrains.kotlin.psi.KtProperty
import org.jetbrains.kotlin.psi.psiUtil.containingClass
import org.jetbrains.kotlin.psi.psiUtil.containingClassOrObject
import org.jetbrains.kotlin.psi.psiUtil.isPublic

/**
Expand All @@ -28,8 +30,8 @@ class UndocumentedPublicProperty(config: Config = Config.empty) : Rule(config) {
"Public properties require documentation.", Debt.TWENTY_MINS)

override fun visitPrimaryConstructor(constructor: KtPrimaryConstructor) {
if (constructor.containingClass()?.isPublic == true) {
val comment = constructor.getContainingClassOrObject().docComment?.text
if (constructor.isPublicInherited()) {
val comment = constructor.containingClassOrObject?.docComment?.text
constructor.valueParameters
.filter { it.isPublicNotOverridden() && it.hasValOrVar() && it.isUndocumented(comment) }
.forEach { report(it) }
Expand All @@ -38,7 +40,7 @@ class UndocumentedPublicProperty(config: Config = Config.empty) : Rule(config) {
}

override fun visitProperty(property: KtProperty) {
if (!property.isLocal && property.docComment == null && property.shouldBeDocumented()) {
if (property.isPublicInherited() && !property.isLocal && property.shouldBeDocumented()) {
report(property)
}
super.visitProperty(property)
Expand All @@ -53,7 +55,9 @@ class UndocumentedPublicProperty(config: Config = Config.empty) : Rule(config) {
}

private fun KtProperty.shouldBeDocumented() =
(isTopLevel || containingClass()?.isPublic == true) && isPublicNotOverridden()
docComment == null && isTopLevelOrInPublicClass() && isPublicNotOverridden()

private fun KtProperty.isTopLevelOrInPublicClass() = isTopLevel || containingClass()?.isPublic == true

private fun report(property: KtNamedDeclaration) {
report(
Expand Down
130 changes: 129 additions & 1 deletion .../kotlin/io/gitlab/arturbosch/detekt/rules/documentation/UndocumentedPublicPropertySpec.kt
View file
Open in desktop
Expand Up @@ -51,14 +51,30 @@ class UndocumentedPublicPropertySpec : Spek({
val code = """
class Test {
/**
*
* Comment
*/
schalkms marked this conversation as resolved.
Show resolved Hide resolved
val a = 1
}
"""
assertThat(subject.compileAndLint(code)).isEmpty()
}

it("does not report undocumented, public and overridden property in class") {
val code = """
interface I {
/**
* Comment
*/
val a: Int
}

class Test : I {
override val a = 1
}
"""
assertThat(subject.compileAndLint(code)).isEmpty()
}

it("does not report undocumented internal and private property") {
val code = """
class Test {
Expand Down Expand Up @@ -132,5 +148,117 @@ class UndocumentedPublicPropertySpec : Spek({
"""
assertThat(subject.compileAndLint(code)).isEmpty()
}

describe("public properties in nested classes") {

it("reports undocumented public properties in nested classes") {
val code = """
class Outer {
class Inner {
val i = 0

class InnerInner {
val ii = 0
}
}
}
"""
assertThat(subject.compileAndLint(code)).hasSize(2)
}

it("reports undocumented public properties in inner classes") {
val code = """
class Outer {
inner class Inner {
val i = 0
}
}
"""
assertThat(subject.compileAndLint(code)).hasSize(1)
}

it("reports undocumented public properties inside objects") {
val code = """
object Outer {
class Inner {
val i = 0
}
}
"""
assertThat(subject.compileAndLint(code)).hasSize(1)
}

it("does not report undocumented and non-public properties in nested classes") {
val code = """
internal class Outer {
class Inner {
val i = 0
}
}
"""
assertThat(subject.compileAndLint(code)).isEmpty()
}

it("does not report undocumented and non-public properties in inner classes") {
val code = """
internal class Outer {
inner class Inner {
val i = 0
}
}
"""
assertThat(subject.compileAndLint(code)).isEmpty()
}
}

describe("public properties in primary constructors inside nested classes") {

it("reports undocumented public properties in nested classes") {
val code = """
class Outer(val a: Int) {
class Inner(val b: Int) {
class InnerInner(val c: Int)
}
}
"""
assertThat(subject.compileAndLint(code)).hasSize(3)
}

it("reports undocumented public properties in inner classes") {
val code = """
class Outer(val a: Int) {
inner class Inner(val b: Int)
}
"""
assertThat(subject.compileAndLint(code)).hasSize(2)
}

it("reports undocumented public properties inside objects") {
val code = """
object Outer {
class Inner(val a: Int)
}
"""
assertThat(subject.compileAndLint(code)).hasSize(1)
}

it("does not report undocumented and non-public properties in nested classes") {
val code = """
internal class Outer(val a: Int) {
class Inner(val b: Int)
}
"""
assertThat(subject.compileAndLint(code)).isEmpty()
}

it("does not report undocumented and non-public properties in inner classes") {
val code = """
internal class Outer(val a: Int) {
inner class Inner(val b: Int)
}
"""
assertThat(subject.compileAndLint(code)).isEmpty()
}
}
}
})