https://kotlinlang.org logo
Title
d

dimsuz

05/19/2022, 12:53 PM
We have released a set of Detekt rules which help us find common errors while working on Compose code. For now there are few rules there, which seem more or less generally applicable (of course, they can be turned on/off individually if they don't work for you): https://github.com/appKODE/detekt-rules-compose
🎉 3
f

Filip Wiesner

05/19/2022, 1:00 PM
ModifierArgumentPosition
and
ModifierParameterPosition
ensure that
modifier
is declared and passed as a first parameter/argument
Why would you want Modifier being passed as a first argument to a Composable? 🤔 I personally pass it always as a last parameter because some modifier chains are long and I want to see the "more important" model parameters first. Nothing against your conventions but I would be interested in the reasoning.
2
d

dimsuz

05/19/2022, 1:04 PM
This seems to be the convention which Material components also use. I guess this was inspired by that and we had this from the start. I thought about whether to include these rules, but then decided that they can be turned off if not desirable... Alternatively I guess we can also provide an option to that rule which either forces it to be the first argument or the last one: those seem to be the most commonly used presets.
f

Filip Wiesner

05/19/2022, 1:09 PM
Yeah, I've been looking through the material components and I see it. Weird, I've never noticed it 😅 And i agree that it's better to have it with ability to turn it off rather than not having it at all. I was just surprised that someone would make a lint about something I explicitly try not to do 😄 Anyways, thank you for making the rules public. I will consider using them 👍 Especially the
ReusedModifierInstance
is nice.
d

dimsuz

05/19/2022, 1:12 PM
Yeah, we had a lot of inconsistencies in larger teams where everyone was placing modifier parameters randomly, so we've decided to stick to something 🙂 Thank you, please report any issues or suggestions!
ReusedModifierInstance
is indeed the most useful one of those. Others are more or less minor, but occasionally helpful.
a

Adam Powell

05/19/2022, 1:20 PM
The official convention is that the modifier param is the first optional parameter in the list. The idea is that you never have to type
modifier =
in the parameter list, it can always be specified positionally if the only other parameters you're using are required
d

dimsuz

05/19/2022, 1:22 PM
Yep, this check only checks the calls/declarations which have it. I.e. if no
modifier
is mentioned, it won't be enforced at all. This is just to stay consistent when it's used.
f

Filip Wiesner

05/19/2022, 1:22 PM
Yes, that is the composable definition. But is there a convention on position of modifier parameter when using the composable? I 100% agree with the composable definition parameter order convention
a

Adam Powell

05/19/2022, 1:24 PM
When calling it the convention is to match the parameter order of the function definition, but I'd probably shake my head at anyone trying to formally enforce that 🙂
Doubly so at someone trying to formally enforce something other than the defined order of the function
If you're using an order other than the function definition's, you're also using named parameters at the call site so making further rules around it can't really use misunderstanding for a reader as a reason
d

dimsuz

05/19/2022, 1:28 PM
🙂 Indeed the one which checks an argument order at the call site is probably not as useful (I actually don't remember it ever firing), perhaps it should be removed. I guess it was conceived out of calls for perfect symmetry :)
f

Filip Wiesner

05/19/2022, 1:33 PM
I wouldn't mind having a rule to enforce using named parameters 😄 🙃 But yeah, I asked about it in the office and we didn't agree on anything. Someone wants it as first parameter, it does not matter for others and I like it as a last parameter. So it's probably highly subjective and situational.
s

ste

05/19/2022, 1:58 PM
Personally, I put
modifier
as the first optional parameter (this can be arguable btw) when it comes to signature, but I always pass it as last (but before any
@Composable
param) when it comes to function invocation - I completely agree with Filip.
♥️ 1
d

dimsuz

05/19/2022, 2:22 PM
Agreed. I will remove the
ModifierArgumentPosition
then and leave only the one which enforces the parameter:
ModifierParameterPosition
. It's also arguably subjective, but can be enabled/disabled depending on codebase conventions.
Following the comments of Adam and ste, will also update
ModifierParameterPosition
to check that
modifier
is the first optional parameter, rather than always-first as it is now.
a

Adam Powell

05/19/2022, 2:41 PM
We considered modifier always first but the counterexample is that having to specify the named parameter in
Text(text = "hello")
as opposed to
Text("hello")
is silly
2
h

hfhbd

05/23/2022, 9:14 AM
Notice to myself: Never use
var showButton = remember { mutableStateOf(false) }.value
. Although
.value
is nice for
val
, because it supports data flow analysis (eg smart casts), with
var
the state won't update 🤦‍♂️ Adding this rule would be nice too 😄
d

dimsuz

05/23/2022, 11:24 AM
I wonder how widespread this error is?... by the way, you can always use
var showButton by remember { ... }
(note the
by
)
h

hfhbd

05/23/2022, 11:28 AM
Yes, you could use
by
, beside
val (showButton, setValue) =
. In my codebase, I found it only once, but it easy to make the same mistake again, I think.