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

Allow users and rule authors to specify a reason per configured value #4611

Merged
merged 28 commits into from Jun 1, 2022
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
60023f3
implement first idea on values with reasons
marschwar Jan 20, 2022
83d9398
Merge remote-tracking branch 'upstream/main' into feature/values-with…
Jan 22, 2022
a736879
have ExplainedValues implement List by delegation
Jan 23, 2022
ea85090
Merge remote-tracking branch 'upstream/main' into feature/values-with…
Jan 23, 2022
fc68d64
Merge remote-tracking branch 'upstream/main' into feature/values-with…
Feb 1, 2022
3f14934
Parse explained value defaults
Feb 9, 2022
6cbb99f
Merge remote-tracking branch 'upstream/main' into feature/values-with…
Feb 9, 2022
1de89d6
fix error message
Feb 9, 2022
304e23a
Merge remote-tracking branch 'upstream/main' into feature/values-with…
marschwar Feb 18, 2022
0bbcdc6
[WIP] support ExplainedValues in yaml
marschwar Feb 18, 2022
0991416
Merge remote-tracking branch 'upstream/main' into feature/values-with…
Mar 5, 2022
f19b593
print markdown value in DefaultValue directly
Mar 5, 2022
5544c0d
quote values in yaml maps
Mar 6, 2022
87f1ea5
Update detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/Con…
marschwar Mar 7, 2022
d8010b6
Update detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/Con…
marschwar Mar 7, 2022
8b24d41
add test case for missing reason
Mar 7, 2022
bd0261e
fail fast in case of invalid configuration
Mar 7, 2022
0156a4c
Merge branch 'main' into feature/values-with-reasons
Mar 21, 2022
204aaec
rename explainedValues to valuesWithReason
Mar 21, 2022
c2eec92
add api documentation
Mar 21, 2022
edcf8c3
resolve and suppress code smells
Mar 21, 2022
ecbce9f
suppress UNCHECKED_CAST
Mar 21, 2022
c0b1038
Merge remote-tracking branch 'upstream/main' into feature/values-with…
Apr 21, 2022
fe99851
Merge remote-tracking branch 'upstream/main' into feature/values-with…
May 22, 2022
8b4a23c
merge RuleCollectorSpec
May 24, 2022
23efa5a
reformat spec
May 24, 2022
4d2d279
fix indentation of test code
May 29, 2022
b27a563
Merge remote-tracking branch 'upstream/main' into feature/values-with…
May 29, 2022
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
Expand Up @@ -111,14 +111,8 @@ fun <T : Any, U : Any> configWithAndroidVariants(
private fun <T : Any> getValueOrDefault(configAware: ConfigAware, propertyName: String, defaultValue: T): T {
@Suppress("UNCHECKED_CAST")
return when (defaultValue) {
is List<*> -> {
if (defaultValue.all { it is String }) {
val defaultValueAsListOfStrings = defaultValue as List<String>
configAware.valueOrDefaultCommaSeparated(propertyName, defaultValueAsListOfStrings) as T
} else {
error("Only lists of strings are supported. '$propertyName' is invalid. ")
}
}
is ExplainedValues -> configAware.getExplainedValuesOrDefault(propertyName, defaultValue) as T
is List<*> -> configAware.getListOrDefault(propertyName, defaultValue) as T
is String,
is Boolean,
is Int -> configAware.valueOrDefault(propertyName, defaultValue)
Expand All @@ -129,6 +123,38 @@ private fun <T : Any> getValueOrDefault(configAware: ConfigAware, propertyName:
}
}

private fun ConfigAware.getListOrDefault(propertyName: String, defaultValue: List<*>): List<String> {
return if (defaultValue.all { it is String }) {
val defaultValueAsListOfStrings = defaultValue as List<String>
valueOrDefaultCommaSeparated(propertyName, defaultValueAsListOfStrings)
} else {
error("Only lists of strings are supported. '$propertyName' is invalid. ")
}
}

private fun ConfigAware.getExplainedValuesOrDefault(
propertyName: String,
defaultValue: ExplainedValues
): ExplainedValues {
val valuesAsList: List<*> = valueOrNull(propertyName) ?: return defaultValue
if (valuesAsList.all { it is String }) {
return ExplainedValues(values = valuesAsList.map { ExplainedValue(it.toString()) })
marschwar marked this conversation as resolved.
Show resolved Hide resolved
}
if (valuesAsList.all { it is Map<*, *> }) {
return ExplainedValues(
valuesAsList
.map { it as Map<*, *> }
.map { dict ->
ExplainedValue(
value = dict["value"].toString(),
reason = dict["reason"]?.toString()
marschwar marked this conversation as resolved.
Show resolved Hide resolved
)
}
)
}
error("Only lists of strings or maps with keys 'value' and 'reason' are supported. '$propertyName' is invalid. ")
marschwar marked this conversation as resolved.
Show resolved Hide resolved
}

private abstract class MemoizedConfigProperty<U : Any> : ReadOnlyProperty<ConfigAware, U> {
private var value: U? = null

Expand Down
@@ -0,0 +1,9 @@
package io.gitlab.arturbosch.detekt.api

fun explainedValues(vararg values: Pair<String, String?>): ExplainedValues {
return ExplainedValues(values.map { ExplainedValue(it.first, it.second) })
}

data class ExplainedValues(val values: List<ExplainedValue>) : List<ExplainedValue> by values

data class ExplainedValue(val value: String, val reason: String? = null)
Expand Up @@ -3,6 +3,7 @@ package io.gitlab.arturbosch.detekt.api
import io.gitlab.arturbosch.detekt.test.TestConfig
import org.assertj.core.api.Assertions.assertThat
import org.assertj.core.api.Assertions.assertThatThrownBy
import org.assertj.core.api.Assertions.tuple
import org.junit.jupiter.api.Nested
import org.junit.jupiter.api.Test
import java.util.concurrent.atomic.AtomicInteger
Expand Down Expand Up @@ -145,6 +146,56 @@ class ConfigPropertySpec {
}
}
}

@Nested
inner class `ExplainedValues property` {
private val defaultValue = explainedValues("aValue" to "aReason")

@Nested
inner class `value defined as list` {
private val subject = object : TestConfigAware("present" to listOf("a", "b", "c")) {
val present: ExplainedValues by config(defaultValue)
val notPresent: ExplainedValues by config(defaultValue)
}

@Test
fun `uses the value provided in config if present`() {
assertThat(subject.present)
.extracting(ExplainedValue::value, ExplainedValue::reason)
.containsExactly(tuple("a", null), tuple("b", null), tuple("c", null))
}

@Test
fun `uses the default value if not present`() {
assertThat(subject.notPresent).isEqualTo(defaultValue)
}
}

@Nested
inner class `value defined as list of maps` {
private val subject = object : TestConfigAware(
"present" to listOf(
mapOf("value" to "a", "reason" to "reasonA"),
mapOf("value" to "b", "reason" to null)
marschwar marked this conversation as resolved.
Show resolved Hide resolved
)
) {
val present: ExplainedValues by config(defaultValue)
val notPresent: ExplainedValues by config(defaultValue)
}

@Test
fun `uses the value provided in config if present`() {
assertThat(subject.present)
.extracting(ExplainedValue::value, ExplainedValue::reason)
.containsExactly(tuple("a", "reasonA"), tuple("b", null))
}

@Test
fun `uses the default value if not present`() {
assertThat(subject.notPresent).isEqualTo(defaultValue)
}
}
}
}

@Nested
Expand Down
Expand Up @@ -126,6 +126,46 @@ class YamlConfigSpec {
assertThat(config).isNotNull
}

@Nested
inner class `explained values` {
private val config = YamlConfig.load(resourceAsPath("explained-values.yml"))

@Test
fun `can be parsed`() {
assertThat(config).isNotNull
}

@Test
fun `supports lists`() {
val actualAsList: List<*>? = config
.subConfig("style")
.subConfig("AsList")
.valueOrNull("values")
assertThat(actualAsList).hasSize(3)
}

@Test
fun `supports dictionaries`() {
val actualAsMap: List<Map<*, *>>? = config
.subConfig("style")
.subConfig("AsListOfMaps")
.valueOrNull("values")
assertThat(actualAsMap)
.hasSize(3)
}

@Test
fun `supports empty dictionaries`() {
val actualAsMap: List<Map<*, *>>? = config
.subConfig("style")
.subConfig("EmptyListOfMaps")
.valueOrNull("values")
assertThat(actualAsMap)
.isNotNull
.isEmpty()
}
}

@Test
fun `throws an exception on an non-existing file`() {
val path = Paths.get("doesNotExist.yml")
Expand Down
15 changes: 15 additions & 0 deletions detekt-core/src/test/resources/explained-values.yml
@@ -0,0 +1,15 @@
style:
AsList:
values:
- a
- b
- c
AsListOfMaps:
values:
- value: a
reason: reason A
- value: b
- value: c
reason: reason C
EmptyListOfMaps:
values: []
Expand Up @@ -8,8 +8,4 @@ data class Configuration(
val deprecated: String?
) {
fun isDeprecated() = deprecated != null

fun isDefaultValueNonEmptyList() = defaultValue.isNonEmptyList()

fun getDefaultValueAsList(): List<String> = defaultValue.getAsList()
}
@@ -1,5 +1,7 @@
package io.gitlab.arturbosch.detekt.generator.collection

import io.gitlab.arturbosch.detekt.api.ExplainedValue
import io.gitlab.arturbosch.detekt.api.ExplainedValues
import io.gitlab.arturbosch.detekt.generator.collection.ConfigurationCollector.ConfigWithAndroidVariantsSupport.ANDROID_VARIANTS_DELEGATE_NAME
import io.gitlab.arturbosch.detekt.generator.collection.ConfigurationCollector.ConfigWithAndroidVariantsSupport.DEFAULT_ANDROID_VALUE_ARGUMENT_NAME
import io.gitlab.arturbosch.detekt.generator.collection.ConfigurationCollector.ConfigWithAndroidVariantsSupport.isAndroidVariantConfigDelegate
Expand All @@ -9,7 +11,12 @@ import io.gitlab.arturbosch.detekt.generator.collection.ConfigurationCollector.C
import io.gitlab.arturbosch.detekt.generator.collection.ConfigurationCollector.DefaultValueSupport.getAndroidDefaultValue
import io.gitlab.arturbosch.detekt.generator.collection.ConfigurationCollector.DefaultValueSupport.getDefaultValue
import io.gitlab.arturbosch.detekt.generator.collection.ConfigurationCollector.DefaultValueSupport.toDefaultValueIfLiteral
import io.gitlab.arturbosch.detekt.generator.collection.ConfigurationCollector.ExplainedValuesSupport.getExplainedValuesDefaultOrNull
import io.gitlab.arturbosch.detekt.generator.collection.ConfigurationCollector.ExplainedValuesSupport.hasExplainedValueDeclaration
import io.gitlab.arturbosch.detekt.generator.collection.ConfigurationCollector.StringListSupport.getListDefaultOrNull
import io.gitlab.arturbosch.detekt.generator.collection.ConfigurationCollector.StringListSupport.hasListDeclaration
import io.gitlab.arturbosch.detekt.generator.collection.exception.InvalidDocumentationException
import org.jetbrains.kotlin.psi.KtBinaryExpression
import org.jetbrains.kotlin.psi.KtCallExpression
import org.jetbrains.kotlin.psi.KtCallableReferenceExpression
import org.jetbrains.kotlin.psi.KtConstantExpression
Expand Down Expand Up @@ -56,8 +63,13 @@ class ConfigurationCollector {
}

private fun KtProperty.getConstantValue(): DefaultValue? {
if (hasExplainedValueDeclaration()) {
return getExplainedValuesDefaultOrNull()
?: invalidDocumentation { "Invalid declaration of explained values default for property '$text'" }
}
if (hasListDeclaration()) {
return DefaultValue.of(getListDeclaration().valueArguments.map { it.text.withoutQuotes() })
return getListDefaultOrNull(emptyMap())
?: invalidDocumentation { "Invalid declaration of string list default for property '$text'" }
}

return findDescendantOfType<KtConstantExpression>()?.toDefaultValueIfLiteral()
Expand Down Expand Up @@ -125,15 +137,9 @@ class ConfigurationCollector {
}

fun KtExpression.toDefaultValue(constantsByName: Map<String, DefaultValue>): DefaultValue {
val listDeclarationForDefault = getListDeclarationOrNull()
if (listDeclarationForDefault != null) {
val listValues = listDeclarationForDefault.valueArguments.map {
(constantsByName[it.text]?.getAsPlainString() ?: it.text.withoutQuotes())
}
return DefaultValue.of(listValues)
}

return toDefaultValueIfLiteral()
return getExplainedValuesDefaultOrNull()
?: getListDefaultOrNull(constantsByName)
?: toDefaultValueIfLiteral()
?: constantsByName[text.withoutQuotes()]
?: error("$text is neither a literal nor a constant")
}
Expand Down Expand Up @@ -176,6 +182,61 @@ class ConfigurationCollector {
delegate?.expression?.referenceExpression()?.text == ANDROID_VARIANTS_DELEGATE_NAME
}

private object ExplainedValuesSupport {
private const val EXPLAINED_VALUE_FACTORY_METHOD = "explainedValues"

fun KtElement.getExplainedValuesDefaultOrNull(): DefaultValue? {
return getExplainedValueDeclarationOrNull()
?.valueArguments
?.map(::toExplainedValue)
?.let { DefaultValue.of(ExplainedValues(it)) }
}

fun KtElement.getExplainedValueDeclarationOrNull(): KtCallExpression? =
findDescendantOfType { it.isExplainedValueDeclaration() }

fun KtCallExpression.isExplainedValueDeclaration(): Boolean {
return referenceExpression()?.text == EXPLAINED_VALUE_FACTORY_METHOD
}

fun KtProperty.hasExplainedValueDeclaration(): Boolean =
anyDescendantOfType<KtCallExpression> { it.isExplainedValueDeclaration() }

private fun toExplainedValue(arg: KtValueArgument): ExplainedValue {
val keyToValue = arg.children.first() as? KtBinaryExpression
return keyToValue?.let {
ExplainedValue(
value = it.left!!.text.withoutQuotes(),
reason = it.right!!.text.withoutQuotes()
)
} ?: error("invalid value argument '${arg.text}'")
}
}

private object StringListSupport {
private const val LIST_OF = "listOf"
private const val EMPTY_LIST = "emptyList"
private val LIST_CREATORS = setOf(LIST_OF, EMPTY_LIST)

fun KtElement.getListDefaultOrNull(constantsByName: Map<String, DefaultValue>): DefaultValue? {
return getListDeclarationOrNull()?.valueArguments?.map {
(constantsByName[it.text]?.getPlainValue() ?: it.text.withoutQuotes())
}?.let { DefaultValue.of(it) }
}

fun KtElement.getListDeclarationOrNull(): KtCallExpression? =
findDescendantOfType { it.isListDeclaration() }

fun KtProperty.hasListDeclaration(): Boolean =
anyDescendantOfType<KtCallExpression> { it.isListDeclaration() }

fun KtElement.getListDeclaration(): KtCallExpression =
checkNotNull(getListDeclarationOrNull())

fun KtCallExpression.isListDeclaration() =
referenceExpression()?.text in LIST_CREATORS
}

companion object {
private const val SIMPLE_DELEGATE_NAME = "config"
private val DELEGATE_NAMES = listOf(
Expand All @@ -184,25 +245,10 @@ class ConfigurationCollector {
ANDROID_VARIANTS_DELEGATE_NAME
)
private const val DEFAULT_VALUE_ARGUMENT_NAME = "defaultValue"
private const val LIST_OF = "listOf"
private const val EMPTY_LIST = "emptyList"
private val LIST_CREATORS = setOf(LIST_OF, EMPTY_LIST)

private fun KtElement.getListDeclaration(): KtCallExpression =
checkNotNull(getListDeclarationOrNull())

private fun KtElement.getListDeclarationOrNull(): KtCallExpression? =
findDescendantOfType { it.isListDeclaration() }

private fun KtProperty.isInitializedWithConfigDelegate(): Boolean =
delegate?.expression?.referenceExpression()?.text in DELEGATE_NAMES

private fun KtProperty.hasListDeclaration(): Boolean =
anyDescendantOfType<KtCallExpression> { it.isListDeclaration() }

private fun KtCallExpression.isListDeclaration() =
referenceExpression()?.text in LIST_CREATORS

private fun KtElement.invalidDocumentation(message: () -> String): Nothing {
throw InvalidDocumentationException("[${containingFile.name}] ${message.invoke()}")
}
Expand Down