appKODE / detekt-rules-compose

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

False positive on ComposableFunctionName when using single expression functions #35

Open patrickhoette opened 5 months ago

patrickhoette commented 5 months ago

I get a false positive on this rule when using the following syntax:

// Part of theme file
@Composable
fun AppTheme(content: @Composable () -> Unit) {
    // Theme setup stuff goes here
}

// False positive is on this function
@Preview
@Composable
private fun PreviewSomeComposable() = AppTheme {
    SomeComposable()
}

Seems like the rule doesn't detect that its still only a Unit return type so wants me to start the function with a lowercase letter.

dimsuz commented 5 months ago

Yeah, we have discussed this here and the conclusion was that this is not something that's usually done in Сompose code, so we didn't bother.

Do you use this somewhere other than in @Preview functions?

If not maybe just add

ignoreAnnotated: [ 'Preview' ]

to this rule's config.

Detecting all cases without rewriting the rule to use type resolution can be tricky, I will check if this can be done without doing that.

patrickhoette commented 5 months ago

I actually use this all the time as it's (IMO) nicer to read and also helps reduce the visual nesting a bit which Compose can get a bit heavy on. The example given in the comment you linked is actually one that I do use quite often. For now we settled on just disabling the rule entirely, but it is one I would like to have enforced. Unfortunately I have no statistics on how common using this syntax is so if it is deemed as uncommon enough to not bother handling it then we will have to discuss internally if we want to stop using this notation in Compose code so we can re-enable the rule.

If an easy solution can be found that would be great. Otherwise maybe I can find some time to look into rewriting the rule to use type resolution. I can't give a guarantee on that however as I don't have a whole lot of free time at the moment.

Thanks for looking into this issue.

dimsuz commented 5 months ago

I see! I will try to improve this rule to account for this case and maybe rewrite to type resolution, it shouldn't be complex, it's just that I don't do that unless necessary - to keep the rules more lightweight and faster. No worries about the PR, but they are welcome :)