appKODE / detekt-rules-compose

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

ComposableParametersOrdering: Ignore trailing lambda parameters #12

Closed leinardi closed 2 years ago

leinardi commented 2 years ago

Currently this rule does not take in consideration the trailing lambda syntax like, for example, in this composable:

@Composable
fun DatePicker(
    show: Boolean,
    selection: Long? = null,
    calendarConstraints: CalendarConstraints? = null,
    title: String? = null,
    @MaterialDatePicker.InputMode inputMode: Int = MaterialDatePicker.INPUT_MODE_CALENDAR,
    fragmentTag: String = "MaterialDatePicker",
    onNegativeButtonClick: (() -> Unit)? = null,
    onCancel: (() -> Unit)? = null,
    onDismiss: (() -> Unit)? = null,
    onPositiveButtonClick: (Long) -> Unit,
) {}

The parameter onPositiveButtonClick not optional but it's the final parameter of the function to be able to pass the lambda outside of the function call parentheses.

dimsuz commented 2 years ago

This rule detects such cases when the trailing lambda is a @Composable function — in this case it doesn't check such functions (no sane way to do that), but it doesn't skip non-composable trailing labmdas, because that's what Compose seems to do too: material components always have their onClick, onValueChange, to follow this rule: they always go with the required parameters at the top.

@Composable
fun Button(
    onClick: () -> Unit,
    modifier: Modifier = Modifier,
    enabled: Boolean = true,
    ...
)

@Composable
fun Slider(
    value: Float,
    onValueChange: (Float) -> Unit,
    modifier: Modifier = Modifier,
    enabled: Boolean = true,
    ...
)

etc.

I'm not sure this rule should be changed to always allow required trailing lambdas to come last.

Perhaps a config option could be added for that.

But this also raises the question: what if there are multiple required lambdas? Only one of them should be allowed to come last, the rest are checked as though it wasn't there?

leinardi commented 2 years ago

Yeah good point, maybe I should just move onPositiveButtonClick up. Btw, thank you for the plugin, these rules are very useful :+1:

dimsuz commented 2 years ago

Thank you!

If this starts to come up in your (and others') codebases very often, maybe we should revisit this!

dimsuz commented 2 years ago

Oh, and you can also put @Suppress("ComposableParametersOrdering") on this individual function if this particular case seems like an exception to the general rule :)