-
Notifications
You must be signed in to change notification settings - Fork 26
[#541] Integrate the Twitter Jetpack Compose Rules Detekt plugin #556
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
base: develop
Are you sure you want to change the base?
[#541] Integrate the Twitter Jetpack Compose Rules Detekt plugin #556
Conversation
Kover report for template-compose:🧛 Template - Compose Unit Tests Code Coverage:
|
| File | Coverage |
|---|---|
HomeScreen.kt |
61.42% |
HomeViewModel.kt |
100.00% |
Modified Files Not Found In Coverage Report:
Dependencies.kt
ItemList.kt
Plugins.kt
SecondScreen.kt
ThirdScreen.kt
Versions.kt
Versions.kt
build.gradle.kts
build.gradle.kts
build.gradle.kts
build.gradle.kts
detekt-config.yml
detekt-config.yml
Codebase cunningly covered by count Shroud 🧛
Generated by 🚫 Danger
|
Don't we need to apply these changes to sample @ryan-conway ? |
|
@ryan-conway Conflict 😶🌫️ |
|
IMO, the |
5662c4f to
391946a
Compare
@kaungkhantsoe Updated in bfda1aa
@lydiasama That's a fair point, but since these are new rules introduced in this task it made sense to me to handle them here as well instead of opening another task, what do you think? 🤔 |
| viewModel: SecondViewModel = hiltViewModel(), | ||
| navigator: (destination: BaseDestination) -> Unit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the viewModel param with the default value be located at the end as well?
| viewModel: SecondViewModel = hiltViewModel(), | |
| navigator: (destination: BaseDestination) -> Unit, | |
| navigator: (destination: BaseDestination) -> Unit, | |
| viewModel: SecondViewModel = hiltViewModel(), |
| id: String, | ||
| modifier: Modifier = Modifier, | ||
| onUpdateClick: () -> Unit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please cross-check with some config from https://github.com/appKODE/detekt-rules-compose, which forces us to order the Modifier with the default value at the end. I think that's a good convention to apply as well 🤔
| model: UiModel?, | ||
| viewModel: ThirdViewModel = hiltViewModel(), | ||
| navigator: (destination: BaseDestination) -> Unit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, params with default values to the bottom.
|
|
||
| @Composable | ||
| fun ThirdScreenContent(data: UiModel?) { | ||
| fun ThirdScreenContent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must be private
| isResultOk: Boolean = false, | ||
| viewModel: HomeViewModel = hiltViewModel(), | ||
| navigator: (destination: BaseDestination) -> Unit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
|
@ryan-conway @luongvo I think we should resume this as soon as version catalogs is merged 💪 |
closes #541
What happened 👀
detekt-config.ymlkotlinx.collections.immutabledependency and updated UI lists to useImmutableListsample-composeInsight 📝
Collection(i.e., List, Set, etc.) as unstable, we should update all UI usages of collections to use theirImmutableXcounterpart to reduce unnecessary recomposition.ignoreAnnotated: [ 'Preview' ]to theMagicNumberrule as preview composables typically contain hard-coded values and we don't want these to be flagged 👀Proof Of Work 📹
With incorrect composable name (
homeScreenContent), public@Previewand unstable lists:With fixes applied: