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

implement UnnecessaryLayoutWrapper rule #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sergey-shevtsov
Copy link

The rule reports if the use of Box, Column or Row is not required. For example, in code below, we can easily remove the parent column.

Column {
    Box(modifier = Modifier.size(200.dp)) {
        // Other content
    }
}

import org.jetbrains.kotlin.psi.psiUtil.getChildOfType
import org.jetbrains.kotlin.psi.psiUtil.getChildrenOfType

class UnnecessaryLayoutWrapper(config: Config = Config.empty) : Rule(config) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add examples of compliant and non compliant code (we'll use them later to generate docs).

See example here

@@ -0,0 +1,75 @@
package ru.kode.detekt.rule.compose
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a license header, here's an example


private val layoutNames = setOf("Box", "Column", "Row")

private val unnecessaryExpressions = mutableListOf<KtCallExpression>()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to accumulate expressions and report them in postVisit, you can report them right when you find them, i.e. instead of having

unnecessaryExpressions.add(expression)

just call

report(...)

Detekt will visit all call expressions anyway, so no need to complicate with preVisit, postVisit and state here...

super.visitCallExpression(expression)
}

private fun KtBlockExpression.hasSingleLayout(): Boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: hasSingleLayoutCall?

@@ -22,3 +22,5 @@ compose:
active: false
ComposableFunctionName:
active: true
UnnecessaryLayoutWrapper:
active: true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs to be added to the sample block in the README.md, some people will be copying rules from here (config.yml doesn't work for older detekt versions)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Secondly, this rule must be added to the rule set provider.

val code = """
@Composable
fun Test() {
Box {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should test all supported layout types here. data-driven testing feature can be used here:

https://kotest.io/docs/framework/datatesting/data-driven-testing.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants