From fc2e1ccac41d20e71e1ed03910ac63c9d103b20d Mon Sep 17 00:00:00 2001 From: Nacho Lopez Date: Tue, 27 Sep 2022 11:51:02 +0200 Subject: [PATCH] Fix ComposePreviewPublic to not look into non-public methods `ComposePreviewPublic` was looking into private/internal methods and could have false positives if they used `PreviewParameter`s. This patch changes that behavior and adds a test to prevent regressions. --- .../compose/rules/ComposePreviewPublic.kt | 9 +++++---- .../detekt/ComposePreviewPublicCheckTest.kt | 18 ++++++++++++++++++ .../ktlint/ComposePreviewPublicCheckTest.kt | 17 +++++++++++++++++ 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/rules/common/src/main/kotlin/com/twitter/compose/rules/ComposePreviewPublic.kt b/rules/common/src/main/kotlin/com/twitter/compose/rules/ComposePreviewPublic.kt index 9459ec77..67ef7bf5 100644 --- a/rules/common/src/main/kotlin/com/twitter/compose/rules/ComposePreviewPublic.kt +++ b/rules/common/src/main/kotlin/com/twitter/compose/rules/ComposePreviewPublic.kt @@ -15,11 +15,12 @@ class ComposePreviewPublic : ComposeKtVisitor { override fun visitComposable(function: KtFunction, autoCorrect: Boolean, emitter: Emitter) { // We only want previews if (!function.isPreview) return - // ...that if it is public, none of it's params is tagged as preview - if (function.isPublic && function.valueParameters.none { it.isPreviewParameter }) return - // ...and if it isn't public, all params are tagged as preview - if (!function.isPublic && function.valueParameters.all { it.isPreviewParameter }) return + // We only care about public methods + if (!function.isPublic) return + // If the method is public, none of it's params should be tagged as preview + if (function.valueParameters.none { it.isPreviewParameter }) return + // If we got here, it's a public method in a @Preview composable with a @PreviewParameter parameter emitter.report(function, ComposablesPreviewShouldNotBePublic, true) if (autoCorrect) { function.addModifier(KtTokens.PRIVATE_KEYWORD) diff --git a/rules/detekt/src/test/kotlin/com/twitter/compose/rules/detekt/ComposePreviewPublicCheckTest.kt b/rules/detekt/src/test/kotlin/com/twitter/compose/rules/detekt/ComposePreviewPublicCheckTest.kt index 91f2ed54..1958dc9a 100644 --- a/rules/detekt/src/test/kotlin/com/twitter/compose/rules/detekt/ComposePreviewPublicCheckTest.kt +++ b/rules/detekt/src/test/kotlin/com/twitter/compose/rules/detekt/ComposePreviewPublicCheckTest.kt @@ -57,6 +57,24 @@ class ComposePreviewPublicCheckTest { } } + @Test + fun `passes when a non-public preview composable uses preview params`() { + @Language("kotlin") + val code = + """ + @Preview + @Composable + private fun MyComposable(@PreviewParameter(User::class) user: User) { + } + @Preview + @Composable + internal fun MyComposable(@PreviewParameter(User::class) user: User) { + } + """.trimIndent() + val errors = rule.lint(code) + assertThat(errors).isEmpty() + } + @Test fun `passes when a private preview composable uses preview params`() { @Language("kotlin") diff --git a/rules/ktlint/src/test/kotlin/com/twitter/compose/rules/ktlint/ComposePreviewPublicCheckTest.kt b/rules/ktlint/src/test/kotlin/com/twitter/compose/rules/ktlint/ComposePreviewPublicCheckTest.kt index 440f5e09..74a615c6 100644 --- a/rules/ktlint/src/test/kotlin/com/twitter/compose/rules/ktlint/ComposePreviewPublicCheckTest.kt +++ b/rules/ktlint/src/test/kotlin/com/twitter/compose/rules/ktlint/ComposePreviewPublicCheckTest.kt @@ -51,6 +51,23 @@ class ComposePreviewPublicCheckTest { ) } + @Test + fun `passes when a non-public preview composable uses preview params`() { + @Language("kotlin") + val code = + """ + @Preview + @Composable + private fun MyComposable(@PreviewParameter(User::class) user: User) { + } + @Preview + @Composable + internal fun MyComposable(@PreviewParameter(User::class) user: User) { + } + """.trimIndent() + ruleAssertThat(code).hasNoLintViolations() + } + @Test fun `autofix makes private the public preview`() { @Language("kotlin")