-
Notifications
You must be signed in to change notification settings - Fork 9
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
base: master
Are you sure you want to change the base?
Conversation
import org.jetbrains.kotlin.psi.psiUtil.getChildOfType | ||
import org.jetbrains.kotlin.psi.psiUtil.getChildrenOfType | ||
|
||
class UnnecessaryLayoutWrapper(config: Config = Config.empty) : Rule(config) { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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>() |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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
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.