appKODE / detekt-rules-compose

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

Implement ComposeFunctionName #15

Closed BraisGabin closed 2 years ago

BraisGabin commented 2 years ago

Functions that return Unit should start with upper case and if they return something different than Unit they should start with lower case.

I'm not checking the rest of the name as there are already other rules on detekt that check that. But I could change that and do pattern check with all the Composable function, it wouldn't be too difficult.

Closes #10

dimsuz commented 2 years ago

Looks great, thank you! Instant merge :)

Internally we thought about writing this with type resolution, to catch cases like

@Composable
fun myComposableInt() = 3

but I guess this is rare enough to not care for now.

We can update this rule to use type resolution later, if it proves useful.

BraisGabin commented 2 years ago

Looks great, thank you! Instant merge :)

Internally we thought about writing this with type resolution, to catch cases like

@Composable
fun myComposableInt() = 3

but I guess this is rare enough to not care for now.

We can update this rule to use type resolution later, if it proves useful.

That case should be handled correctly my current implementation. The only case that I can think that will fail is this one:

@Composable
fun MyComposable() = Text("Hello there!")

For that code the rule right now will say that MyComposable should be called myComposable but I hope that no one will use that type of notation with composble functions returning Unit. In detekt we even have a rule to avoid that type of code: ImplicitUnitReturnType. It will force you to write: fun MyComposable(): Unit = Text("Hello there!") and in this case this rule will work perfectly.

dimsuz commented 2 years ago

Oh, right! :+1:

One more thought I just had: maybe ComposeFunctionName should become ComposableFunctionName? Compose is the name of the framework, while composable is the kind of the function. But not strictly sure, both make sense it seems.

BraisGabin commented 2 years ago

You are right. Composable feels like a better name to me too.

Kernald commented 2 years ago

That case should be handled correctly my current implementation. The only case that I can think that will fail is this one:

@Composable
fun MyComposable() = Text("Hello there!")

For that code the rule right now will say that MyComposable should be called myComposable but I hope that no one will use that type of notation with composble functions returning Unit. In detekt we even have a rule to avoid that type of code: ImplicitUnitReturnType. It will force you to write: fun MyComposable(): Unit = Text("Hello there!") and in this case this rule will work perfectly.

After enabling this ruleset, I've immediately hit a false positive on this one. My case is a generic list composable, managing the empty state:

@Composable
fun <T : Collection<*>> EmptiableContainer(
  value: T,
  emptyState: @Composable () -> Unit,
  valueState: @Composable (T) -> Unit,
) = when (value.isEmpty) {
  true -> emptyState()
  false -> valueState(value)
}

The return type is fairly obvious here. ImplicitUnitReturnType is also not enabled by default in Detekt.

dimsuz commented 2 years ago

This still looks more like a single exception which can be suppressed with @Suppress. If you have a lot of such generic functions written in "expression body"-style maybe this rule should be turned off to reduce noise.

I guess at some point we will convert it to run only in type-resolution mode and then it will handle your case correctly.