appKODE / detekt-rules-compose

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

False positive for `UnnecessaryEventHandlerParameter`? #33

Closed pm-nchain closed 10 months ago

pm-nchain commented 10 months ago

Following code flags B composable with UnnecessaryEventHandlerParameter, is that OK?

@Composable
private fun Smth(
    state: State,
    onClick: (String) -> Unit,
) {
    Column {
        A(onClick = onClick)
        B(onClick = { onClick(state.string) } )
    }
}

If I would avoid this lint error, I would need to have 2 callbacks, which I think is maybe less readable?

@Composable
private fun Smth(
    state: State,
    onClickA: (String) -> Unit,
    onClickB: () -> Unit,
) {
    Column {
        A(onClick = onClick)
        B(onClick = onClickB)
    }
}

Version: 1.3.0

dimsuz commented 10 months ago

It suggests you to move usage of state.string to the caller, because it seems redundant that you pass state to this composable, only to reference its value and pass it back up to the caller, while caller knows this value even without this call.

Maybe in your case it's really necessary (then I'd suggest to use @Suppress here), but maybe it points to some kind of a design issue.

I can't know your exact use case, but if reacting on A click and reacting on B click cause different outcomes, then it indeed looks like this should be two different handlers.

pm-nchain commented 10 months ago

I think our use case, the String was a website url. So in terms of callback onOpenUrl: (String) -> Unit.

Not sure if that's something you want to take into account. Feel free to close it.

dimsuz commented 10 months ago

Ah, I see! In this case it may be better to have urls in the ViewModel, near where "non-layout" logic resides, making ui rendering code "dumber" :) I usually do something like:

Component(
  onTermsConditionsClick = { viewModel.onTermsConditionsClick() },
  onPrivacyPoliciClick = { viewModel.onPrivacyPolicyClick() }
)

and then ViewModel knows exact URLs which it should open. So indeed, different handlers.

But in a very simple cases you can decide to hardcode in UI, yep. Then maybe @Suppress would be the better solution for this particular code.

dimsuz commented 10 months ago

Will close for now. If this will start to manifest more often for you, please reopen with common use cases, this may help to generalize the correct fix.