detekt-rules-compose icon indicating copy to clipboard operation
detekt-rules-compose copied to clipboard

False positive on `ModifierParameterPosition` and `ComposableParametersOrdering`

Open AfigAliyev opened this issue 3 years ago • 6 comments

@Composable
fun MyComposable(
    modifier: Modifier = Modifier,
    content: LazyListScope.() -> Unit
) = LazyColumn(modifier = modifier, content = content)

ModifierParameterPosition and ComposableParametersOrdering raise an issue. The content parameter should be the last parameter.

AfigAliyev avatar Sep 09 '22 15:09 AfigAliyev

Currently these rules ignore only @Composable trailing lambdas, because non-composable trailing lambdas are usually event handlers and they should follow the optional/required placement rule. We have discussed this in #12.

LazyColumn seems to be an exception here, because it's a DSL for building content, instead of content being a @Composable function.

I'm not sure yet how to best approach this:

  • adding exception for LazyListScope specifically sounds like something flaky, other such "exceptions" may surface later
  • adding exception for any lambda with receiver also sounds like something which could be wrong: there can be some event handlers with scope...

dimsuz avatar Sep 09 '22 16:09 dimsuz

Brainstorming (they are not good ideas, they are just random ideas that maybe help to find a solution):

  • Ignore if the name is content.
  • A configuration with a list of receivers to ignore and add LazyListScope as a default value.
  • A configuration where you need to describe ALL the lambda to be "ignored": LazyListScope.() -> Unit.
  • A boolean configuration to relax this rule. If it is enabled all the trailing lambdas are allowed.

BraisGabin avatar Sep 10 '22 10:09 BraisGabin

I like the second option (ignore specified receivers) and the fourth one (ignore all trailing lambdas). Maybe they both can be added as configuration parameters, although the ignoring all lambdas set to true will effectively override whatever is configured with second option, but this should not be too confusing.

dimsuz avatar Sep 12 '22 11:09 dimsuz

I like the second option too, but I would like to add that it only makes sense to apply it to the last parameter of a function due to the DSL.

AfigAliyev avatar Sep 13 '22 05:09 AfigAliyev