Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend UnnecessaryEventHandlerParameter for constants #3

Open
BraisGabin opened this issue May 31, 2022 · 4 comments
Open

Extend UnnecessaryEventHandlerParameter for constants #3

BraisGabin opened this issue May 31, 2022 · 4 comments

Comments

@BraisGabin
Copy link
Contributor

I think that code like this should be also flagged:

@Composable
private fun Foo(
    onSomething: (Int) -> Unit,
) {
    Button(onClick = { onSomething(5) }) { /*...*/ }
}

When 5 could be any kind of constant value/object/data class with contant values on it... Probably it's impossible to handle ALL the cases but probably the more easy ones.

Context

In my project we use MVI so we start with a lambda like this: onUserIntent: (SealedClassIntent) -> Unit. And we propagate that or something similar. I'm not 100% sure that I like that but it's kind of OK. But what I think it's not Ok is when it ends up to a Component like this:

@Composable
private fun Foo(
    onUserIntent: (SealedClassIntent) -> Unit,
) {
    Button(onClick = { onUserIntent(SealedClassIntent.Back) }) { /*...*/ }
}

I think it should be something like this:

@Composable
private fun Foo(
    onBackClick: () -> Unit,
) {
    Button(onClick = onBackClick) { /*...*/ }
}

And it's the caller job to decide which SealedClassIntent should this Composable emit.

@dimsuz
Copy link
Collaborator

dimsuz commented May 31, 2022

I agree!

It would be easy to implement for primitive types, but it seems that it would require the type resolution enabled for your example with a sealed class? Otherwise it seems a quite difficult task to find out that SealedClassIntent.Back is actualy an object.

@osipxd
Copy link

osipxd commented Aug 14, 2022

Maybe it is possible to ignore the rule for lambda if its parameter is not derived from composable arguments?
For example:

// GOOD. Lambda parameter created inside the composable
@Composable
private fun Foo(
    onSomething: (Any) -> Unit,
) {
    Button(onClick = { onSomething(5) }) { /*...*/ }
}

// BAD. Lambda parameter derived from composable arguments
@Composable
private fun Foo(
    number: Int,
    onSomething: (Int) -> Unit,
) {
    Button(onClick = { onSomething(number) }) { /*...*/ }
}

@dimsuz
Copy link
Collaborator

dimsuz commented Aug 14, 2022

Hmm. But this rule tries to nudge you to change the first case into letting the parent supply the constant while simplifing the implementation of Foo:

fun main() {
  Foo(onSomething = { doStuff(5) })
}

@Composable
private fun Foo(onSomething: () -> Unit) {
  Button(onClick = onSomething)
}

Wouldn't this work for you?

@osipxd
Copy link

osipxd commented Aug 15, 2022

Ah. I misread the issue. Sorry :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants