-
Notifications
You must be signed in to change notification settings - Fork 8
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
Comments
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 |
Maybe it is possible to ignore the rule for lambda if its parameter is not derived from composable arguments? // 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) }) { /*...*/ }
} |
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 fun main() {
Foo(onSomething = { doStuff(5) })
}
@Composable
private fun Foo(onSomething: () -> Unit) {
Button(onClick = onSomething)
} Wouldn't this work for you? |
Ah. I misread the issue. Sorry :) |
I think that code like this should be also flagged:
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:I think it should be something like this:
And it's the caller job to decide which
SealedClassIntent
should thisComposable
emit.The text was updated successfully, but these errors were encountered: