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 configuration to the ModifierMissing rule #33

Merged
merged 1 commit into from
Mar 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/detekt.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ Compose:
active: true
ModifierMissing:
active: true
# You can optionally control the visibility of which composables to check for here
# Possible values are: only_public, public_and_internal_ all (default is only_public)
# checkModifiersForVisibility: only_public
ModifierReused:
active: true
ModifierWithoutDefault:
Expand Down
14 changes: 14 additions & 0 deletions docs/ktlint.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,20 @@ The `compose:naming-check` rule requires all composables that return a value to
compose_allowed_composable_function_names = .*Presenter,.*SomethingElse
```

### Configure the visibility of the composables where to check for missing modifiers

The `compose:modifier-missing-check` rule will, by default, only look for missing modifiers for public composables. If you want to lower the visibility threshold to check also internal compoosables, or all composables, you can configure it in your `.editorconfig` file:

```editorconfig
[*.{kt,kts}]
compose_check_modifiers_for_visibility = only_public
```

Possible values are:
- `only_public`: (default) Will check for missing modifiers only for public composables.
- `public_and_internal`: Will check for missing modifiers in both public and internal composables.
- `all`: Will check for missing modifiers in all composables.

## Disabling a specific rule

To disable a rule you have to follow the [instructions from the ktlint documentation](https://github.com/pinterest/ktlint#how-do-i-suppress-an-errors-for-a-lineblockfile), and use the id of the rule you want to disable with the `compose` tag.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@
// SPDX-License-Identifier: Apache-2.0
package io.nlopez.compose.rules

import io.nlopez.rules.core.ComposeKtConfig.Companion.config
import io.nlopez.rules.core.ComposeKtVisitor
import io.nlopez.rules.core.Emitter
import io.nlopez.rules.core.report
import io.nlopez.rules.core.util.definedInInterface
import io.nlopez.rules.core.util.emitsContent
import io.nlopez.rules.core.util.isInternal
import io.nlopez.rules.core.util.isOverride
import io.nlopez.rules.core.util.isPreview
import io.nlopez.rules.core.util.modifierParameter
Expand All @@ -18,12 +20,10 @@ class ComposeModifierMissing : ComposeKtVisitor {

override fun visitComposable(function: KtFunction, autoCorrect: Boolean, emitter: Emitter) {
// We want to find all composable functions that:
// - are public
// - emit content
// - are not overridden or part of an interface
// - are not a @Preview composable
if (
!function.isPublic ||
function.returnsValue ||
function.isOverride ||
function.definedInInterface ||
Expand All @@ -32,6 +32,21 @@ class ComposeModifierMissing : ComposeKtVisitor {
return
}

// We want to check now the visibility to see whether it's allowed by the configuration
// Possible values:
// - only_public: will check for modifiers only on public composables
// - public_and_internal: will check for public and internal composables
// - all: will check all composables (public, internal, protected, private
val shouldCheck = when (
function.config().getString("checkModifiersForVisibility", "only_public")
) {
"only_public" -> function.isPublic
"public_and_internal" -> function.isPublic || function.isInternal
"all" -> true
else -> function.isPublic
}
if (!shouldCheck) return

// If there is a modifier param, we bail
if (function.modifierParameter != null) return

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
package io.nlopez.compose.rules.detekt

import io.gitlab.arturbosch.detekt.api.Config
import io.gitlab.arturbosch.detekt.api.SourceLocation
import io.gitlab.arturbosch.detekt.test.TestConfig
import io.gitlab.arturbosch.detekt.test.assertThat
import io.gitlab.arturbosch.detekt.test.lint
import io.nlopez.compose.rules.ComposeModifierMissing
Expand Down Expand Up @@ -76,7 +78,7 @@ class ComposeModifierMissingCheckTest {
}

@Test
fun `non-public visibility Composables are ignored`() {
fun `non-public visibility Composables are ignored (by default)`() {
@Language("kotlin")
val code =
"""
Expand Down Expand Up @@ -107,6 +109,94 @@ class ComposeModifierMissingCheckTest {
assertThat(errors).isEmpty()
}

@Test
fun `public and internal visibility Composables are checked for 'public_and_internal' configuration`() {
val newRule = ComposeModifierMissingCheck(
TestConfig("checkModifiersForVisibility" to "public_and_internal"),
)

@Language("kotlin")
val code =
"""
@Composable
fun Something() {
Row {
}
}
@Composable
protected fun Something() {
Column(modifier = Modifier.fillMaxSize()) {
}
}
@Composable
internal fun Something() {
SomethingElse {
Box(modifier = Modifier.fillMaxSize()) {
}
}
}
@Composable
private fun Something() {
Whatever(modifier = Modifier.fillMaxSize()) {
}
}
""".trimIndent()
val errors = newRule.lint(code)
assertThat(errors).hasSize(2)
.hasStartSourceLocations(
SourceLocation(2, 5),
SourceLocation(12, 14),
)
for (error in errors) {
assertThat(error).hasMessage(ComposeModifierMissing.MissingModifierContentComposable)
}
}

@Test
fun `all Composables are checked for 'all' configuration`() {
val newRule = ComposeModifierMissingCheck(
TestConfig("checkModifiersForVisibility" to "all"),
)

@Language("kotlin")
val code =
"""
@Composable
fun Something() {
Row {
}
}
@Composable
protected fun Something() {
Column(modifier = Modifier.fillMaxSize()) {
}
}
@Composable
internal fun Something() {
SomethingElse {
Box(modifier = Modifier.fillMaxSize()) {
}
}
}
@Composable
private fun Something() {
Whatever(modifier = Modifier.fillMaxSize()) {
}
}
""".trimIndent()
val errors = newRule.lint(code)
assertThat(errors).hasSize(4)
.hasStartSourceLocations(
SourceLocation(2, 5),
SourceLocation(7, 15),
SourceLocation(12, 14),
SourceLocation(19, 13),
)
for (error in errors) {
assertThat(error).hasMessage(ComposeModifierMissing.MissingModifierContentComposable)
}
}

@Test
fun `interface Composables are ignored`() {
@Language("kotlin")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,24 @@ val contentEmittersProperty: EditorConfigProperty<String> =
},
)

val checkModifiersForVisibility: EditorConfigProperty<String> =
EditorConfigProperty(
type = PropertyType.LowerCasingPropertyType(
"compose_check_modifiers_for_visibility",
"Visibility of the composables where we want to check if a Modifier parameter is missing",
PropertyValueParser.IDENTITY_VALUE_PARSER,
setOf("only_public", "public_and_internal", "all"),
),
defaultValue = "only_public",
propertyMapper = { property, _ ->
when {
property?.isUnset == true -> "only_public"
property?.getValueAs<String>() != null -> property.getValueAs<String>()
else -> property?.getValueAs()
}
},
)

val compositionLocalAllowlistProperty: EditorConfigProperty<String> =
EditorConfigProperty(
type = PropertyType.LowerCasingPropertyType(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package io.nlopez.compose.rules.ktlint

import com.pinterest.ktlint.test.KtLintAssertThat.Companion.assertThatRule
import com.pinterest.ktlint.test.LintViolation
import io.nlopez.compose.rules.ComposeCompositionLocalAllowlist
import org.intellij.lang.annotations.Language
import org.junit.jupiter.api.Test

Expand All @@ -26,22 +27,22 @@ class ComposeCompositionLocalAllowlistCheckTest {
LintViolation(
line = 1,
col = 13,
detail = io.nlopez.compose.rules.ComposeCompositionLocalAllowlist.CompositionLocalNotInAllowlist,
detail = ComposeCompositionLocalAllowlist.CompositionLocalNotInAllowlist,
),
LintViolation(
line = 2,
col = 14,
detail = io.nlopez.compose.rules.ComposeCompositionLocalAllowlist.CompositionLocalNotInAllowlist,
detail = ComposeCompositionLocalAllowlist.CompositionLocalNotInAllowlist,
),
LintViolation(
line = 3,
col = 5,
detail = io.nlopez.compose.rules.ComposeCompositionLocalAllowlist.CompositionLocalNotInAllowlist,
detail = ComposeCompositionLocalAllowlist.CompositionLocalNotInAllowlist,
),
LintViolation(
line = 4,
col = 13,
detail = io.nlopez.compose.rules.ComposeCompositionLocalAllowlist.CompositionLocalNotInAllowlist,
detail = ComposeCompositionLocalAllowlist.CompositionLocalNotInAllowlist,
),
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class ComposeModifierMissingCheckTest {
}

@Test
fun `non-public visibility Composables are ignored`() {
fun `non-public visibility Composables are ignored (by default)`() {
@Language("kotlin")
val code =
"""
Expand Down Expand Up @@ -125,6 +125,104 @@ class ComposeModifierMissingCheckTest {
modifierRuleAssertThat(code).hasNoLintViolations()
}

@Test
fun `public and internal visibility Composables are checked for 'public_and_internal' configuration`() {
@Language("kotlin")
val code =
"""
@Composable
fun Something() {
Row {
}
}
@Composable
protected fun Something() {
Column(modifier = Modifier.fillMaxSize()) {
}
}
@Composable
internal fun Something() {
SomethingElse {
Box(modifier = Modifier.fillMaxSize()) {
}
}
}
@Composable
private fun Something() {
Whatever(modifier = Modifier.fillMaxSize()) {
}
}
""".trimIndent()
modifierRuleAssertThat(code)
.withEditorConfigOverride(checkModifiersForVisibility to "public_and_internal")
.hasLintViolationsWithoutAutoCorrect(
LintViolation(
line = 2,
col = 5,
detail = ComposeModifierMissing.MissingModifierContentComposable,
),
LintViolation(
line = 12,
col = 14,
detail = ComposeModifierMissing.MissingModifierContentComposable,
),
)
}

@Test
fun `all Composables are checked for 'all' configuration`() {
@Language("kotlin")
val code =
"""
@Composable
fun Something() {
Row {
}
}
@Composable
protected fun Something() {
Column(modifier = Modifier.fillMaxSize()) {
}
}
@Composable
internal fun Something() {
SomethingElse {
Box(modifier = Modifier.fillMaxSize()) {
}
}
}
@Composable
private fun Something() {
Whatever(modifier = Modifier.fillMaxSize()) {
}
}
""".trimIndent()
modifierRuleAssertThat(code)
.withEditorConfigOverride(checkModifiersForVisibility to "all")
.hasLintViolationsWithoutAutoCorrect(
LintViolation(
line = 2,
col = 5,
detail = ComposeModifierMissing.MissingModifierContentComposable,
),
LintViolation(
line = 7,
col = 15,
detail = ComposeModifierMissing.MissingModifierContentComposable,
),
LintViolation(
line = 12,
col = 14,
detail = ComposeModifierMissing.MissingModifierContentComposable,
),
LintViolation(
line = 19,
col = 13,
detail = ComposeModifierMissing.MissingModifierContentComposable,
),
)
}

@Test
fun `interface Composables are ignored`() {
@Language("kotlin")
Expand Down