appKODE / detekt-rules-compose

A collection of Detekt rules for Jetpack Compose
MIT License
136 stars 8 forks source link

ComposableEventParameterNaming false positive #6

Closed raul19 closed 2 years ago

raul19 commented 2 years ago

Expected Behavior

I have a Composable function that receive an extension of LazyListScope lambda as content, these lambda cannot be @Composable because LazyColumn requires that it isn't.

The false positive is that detekt says that the name of the parameter should be like "onClick".

For example, I have a scaffold like:

fun Scaffold(
    topBar: @Composable () -> Unit,
    logo: @Composable BoxScope.() -> Unit,
    content: LazyListScope.(minContentHeight: Dp) -> Unit,
    bottomContent: @Composable ColumnScope.() -> Unit,
    modifier: Modifier = Modifier,
    state: GamificationScaffoldState = rememberGamificationScaffoldState(),
    snackbarHost: @Composable (SnackbarHostState) -> Unit = { SnackbarHost(it) },
) {
    ....
}

Observed Behavior

Detekt says that the content parameter should be named like "onClick" or "onEvent"

Steps to Reproduce

Create a @Composable Scaffold with a non composable content, like LazyListScope.() -> Unit

Context

Your Environment

dimsuz commented 2 years ago

Indeed, this should not be reported as an error.

One solution would be to ignore lambdas with receiver when checking if some function parameter is an "event handler": SomeReceiver.() -> Unit would be excluded then. Although technically you could have onClicked: SomeReceiver.() -> Unit, but I guess this is rare, so if this won't be caught, no big deal.

Another approach would be to watch for a set of predefined names like: parameter named "content" or recever named "LazyRowScope", but I suspect this "predefined names list" won't ever cover all possible cases and it's better to stick to some more generic rules when deciding if some parameter is an "event handler".

BraisGabin commented 2 years ago

I agree about disable it for lambdas with receivers (extension lambdas? does that exist?). Other option is to Ignore the ExtensionLambdas with receivers that end with "Scope" in its name. But as you said probably a more generic solution is better here.

dimsuz commented 2 years ago

Not sure about an official name for such lambdas too :)

A thought about checking for the "Scope" suffix crossed my mind too, but it seems there will be exceptions occasionally. This rule aims to catch the most obvious violations (at least for now), so I'll do it the generic way and we'll see.