mrmans0n / compose-rules

Lint rules for ktlint/detekt aimed to contribute to a healthier usage of Compose. Actively maintained and evolved fork of the Twitter Compose rules.
https://mrmans0n.github.io/compose-rules
Other
525 stars 19 forks source link

Param order check gets confuse when there are many lambdas. #268

Closed lcszc closed 1 month ago

lcszc commented 1 month ago

Hi there!

We're facing some issues while using compose-rules to enforce good practices but param-order-check lint rule seems to be inconsistent or broken in some cases.

For example, given you have this composable

@Composable
fun Drawer(
    items: ImmutableList<Items>, 
    modifier: Modifier = Modifier,
    sheetState: SheetState = rememberModalBottomSheetState(),
    onDismiss: () -> Unit,
    onCloseTapped: () -> Unit,
    onDoneTapped: () -> Unit,
)

IMO the param order seems fair and OK, I see it is better to have lambdas after normal params. I guess even the lint rule also thinks the same, it seems to be somehow random because in some cases it will not complain if you have the param configuration above.

Well, in my case it says:

The params order should be: [items, onDismiss, onCloseTapped, modifier, sheetState, onDoneTapped]

It feels like a bug to push onDismiss and onCloseTapped but onDoneTapped, although I strongly believe it is fine to have lambdas as the last parameters.

Is there a way to avoid this other than ignoring the lint rule?

mrmans0n commented 1 month ago

You have two lambdas there that are non optional after Modifier, which is the first optional item. The trailing one is special so it won't get flagged (as it might be legit to have a content lambdas as last param even if it doesn't have a default).

But in this particular case all 3 lambdas should go before the Modifier, as they are all events and most importantly, they are not optional.

The suggested order from the rule is that : just a suggestion to make the rule pass. But in this case it's encouraged to just move all 3 lambdas to be before the Modifier.

I'll update the messaging so instead of should be It says could be 😄

lcszc commented 1 month ago

It seemed like a bug but your answer is fair enough. Thanks!