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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#541] Integrate the Twitter Jetpack Compose Rules Detekt plugin #556

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

ryan-conway
Copy link
Collaborator

@ryan-conway ryan-conway commented Nov 3, 2023

closes #541

What happened 馃憖

  • Added the compose-rules detekt plugin
  • Updated detekt-config.yml
  • Added kotlinx.collections.immutable dependency and updated UI lists to use ImmutableList
  • Brought detekt rules changes to sample-compose

Insight 馃摑

  • As the Twitter compose-rules detekt plugin seems abandoned, we should instead use a forked version from one of the original maintainers which is being actively worked on 馃檹馃徎
  • Because compose treats all instances of Collection (i.e., List, Set, etc.) as unstable, we should update all UI usages of collections to use their ImmutableX counterpart to reduce unnecessary recomposition.
  • We have added ignoreAnnotated: [ 'Preview' ] to the MagicNumber rule 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 @Preview and unstable lists:

ryanconway@Ryans-MacBook-Pro-13-2020-Nimble template-compose % detekt

> Task :detekt
.....................................................

53 kotlin files were analyzed.
/Users/ryanconway/Projects/Nimble/android-templates/template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/home/HomeScreen.kt:41:13: Composable functions that return Unit should start with an uppercase letter.
They are considered declarative entities that can be either present or absent in a composition and therefore follow the naming rules for classes.

See https://mrmans0n.github.io/compose-rules/rules/#naming-composable-functions-properly for more information. [ComposableNaming]
/Users/ryanconway/Projects/Nimble/android-templates/template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/home/HomeScreen.kt:62:5: Composables annotated with @Preview that are used only for previewing the UI should not be public.

See https://mrmans0n.github.io/compose-rules/rules/#preview-composables-should-not-be-public for more information. [PreviewPublic]
/Users/ryanconway/Projects/Nimble/android-templates/template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/home/HomeScreen.kt:43:15: The Compose Compiler cannot infer the stability of a parameter if a List<UiModel> is used in it, even if the item type is stable.
You should use Kotlinx Immutable Collections instead: `uiModels: ImmutableList<UiModel>` or create an `@Immutable` wrapper for this class: `@Immutable data class UiModelsList(val items: List<UiModel>)`

See https://mrmans0n.github.io/compose-rules/rules/#avoid-using-unstable-collections for more information. [UnstableCollections]

/Users/ryanconway/Projects/Nimble/android-templates/template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/home/HomeScreen.kt - 35min debt
        ComposableNaming - [Composable functions that return Unit should start with an uppercase letter. The(...)] at /Users/ryanconway/Projects/Nimble/android-templates/template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/home/HomeScreen.kt:41:13
        PreviewPublic - [Composables annotated with @Preview that are used only for previewing the UI sho(...)] at /Users/ryanconway/Projects/Nimble/android-templates/template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/home/HomeScreen.kt:62:5
        UnstableCollections - [The Compose Compiler cannot infer the stability of a parameter if a List<UiModel(...)] at /Users/ryanconway/Projects/Nimble/android-templates/template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/home/HomeScreen.kt:43:15

Overall debt: 35min

Compose - 35min debt
        ComposableNaming - [Composable functions that return Unit should start with an uppercase letter. The(...)] at /Users/ryanconway/Projects/Nimble/android-templates/template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/home/HomeScreen.kt:41:13
        PreviewPublic - [Composables annotated with @Preview that are used only for previewing the UI sho(...)] at /Users/ryanconway/Projects/Nimble/android-templates/template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/home/HomeScreen.kt:62:5
        UnstableCollections - [The Compose Compiler cannot infer the stability of a parameter if a List<UiModel(...)] at /Users/ryanconway/Projects/Nimble/android-templates/template-compose/app/src/main/java/co/nimblehq/template/compose/ui/screens/home/HomeScreen.kt:43:15

Overall debt: 35min

Complexity Report:
        - 1,201 lines of code (loc)
        - 915 source lines of code (sloc)
        - 544 logical lines of code (lloc)
        - 22 comment lines of code (cloc)
        - 86 cyclomatic complexity (mcc)
        - 19 cognitive complexity
        - 3 number of total code smells
        - 2% comment source ratio
        - 158 mcc per 1,000 lloc
        - 5 code smells per 1,000 lloc

Project Statistics:
        - number of properties: 145
        - number of functions: 53
        - number of classes: 32
        - number of packages: 23
        - number of kt files: 53

Successfully generated Checkstyle XML report at /Users/ryanconway/Projects/Nimble/android-templates/template-compose/build/reports/detekt/detekt.xml
Successfully generated plain text report at /Users/ryanconway/Projects/Nimble/android-templates/template-compose/build/reports/detekt/detekt.txt
Successfully generated HTML report at /Users/ryanconway/Projects/Nimble/android-templates/template-compose/build/reports/detekt/detekt.html
Successfully generated SARIF: a standard format for the output of static analysis tools at /Users/ryanconway/Projects/Nimble/android-templates/template-compose/build/reports/detekt/detekt.sarif
Successfully generated Markdown report at /Users/ryanconway/Projects/Nimble/android-templates/template-compose/build/reports/detekt/detekt.md

BUILD SUCCESSFUL in 15s
5 actionable tasks: 1 executed, 4 up-to-date

With fixes applied:

ryanconway@Ryans-MacBook-Pro-13-2020-Nimble template-compose % detekt

> Task :detekt
.....................................................

53 kotlin files were analyzed.
Complexity Report:
        - 1,203 lines of code (loc)
        - 917 source lines of code (sloc)
        - 544 logical lines of code (lloc)
        - 22 comment lines of code (cloc)
        - 86 cyclomatic complexity (mcc)
        - 19 cognitive complexity
        - 0 number of total code smells
        - 2% comment source ratio
        - 158 mcc per 1,000 lloc
        - 0 code smells per 1,000 lloc

Project Statistics:
        - number of properties: 145
        - number of functions: 53
        - number of classes: 32
        - number of packages: 23
        - number of kt files: 53

Successfully generated Checkstyle XML report at /Users/ryanconway/Projects/Nimble/android-templates/template-compose/build/reports/detekt/detekt.xml
Successfully generated plain text report at /Users/ryanconway/Projects/Nimble/android-templates/template-compose/build/reports/detekt/detekt.txt
Successfully generated HTML report at /Users/ryanconway/Projects/Nimble/android-templates/template-compose/build/reports/detekt/detekt.html
Successfully generated SARIF: a standard format for the output of static analysis tools at /Users/ryanconway/Projects/Nimble/android-templates/template-compose/build/reports/detekt/detekt.sarif
Successfully generated Markdown report at /Users/ryanconway/Projects/Nimble/android-templates/template-compose/build/reports/detekt/detekt.md

BUILD SUCCESSFUL in 2s
5 actionable tasks: 1 executed, 4 up-to-date

Copy link

github-actions bot commented Nov 3, 2023

2 Warnings
鈿狅笍 Uh oh! HomeScreen.kt is under 95% coverage!
鈿狅笍 Uh oh! Your project is under 80% coverage!

Kover report for template-compose:

馃 Template - Compose Unit Tests Code Coverage: 61.24%

Coverage of Modified Files:

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

@kaungkhantsoe
Copy link
Contributor

Don't we need to apply these changes to sample @ryan-conway ?

@ryan-conway ryan-conway added this to the 3.27.0 milestone Dec 1, 2023
@lydiasama
Copy link
Contributor

@ryan-conway Conflict 馃樁鈥嶐煂笍

@lydiasama
Copy link
Contributor

IMO, the unstable list should be handled in another PR instead. This PR is about to update the Jetpack Compose Rule. 馃 Please correct me if I'm wrong. 馃檱馃徎

@ryan-conway ryan-conway force-pushed the feature/541-integrate-the-twitter-jetpack-compose-rules-detekt-plugin branch from 5662c4f to 391946a Compare December 15, 2023 04:59
@ryan-conway
Copy link
Collaborator Author

Don't we need to apply these changes to sample @ryan-conway ?

@kaungkhantsoe Updated in bfda1aa

IMO, the unstable list should be handled in another PR instead. This PR is about to update the Jetpack Compose Rule. 馃 Please correct me if I'm wrong. 馃檱馃徎

@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? 馃

@ryan-conway ryan-conway modified the milestones: 3.27.0, 3.29.0 Apr 1, 2024
Comment on lines 26 to 27
viewModel: SecondViewModel = hiltViewModel(),
navigator: (destination: BaseDestination) -> Unit,
Copy link
Member

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?

Suggested change
viewModel: SecondViewModel = hiltViewModel(),
navigator: (destination: BaseDestination) -> Unit,
navigator: (destination: BaseDestination) -> Unit,
viewModel: SecondViewModel = hiltViewModel(),

Comment on lines 39 to 41
id: String,
modifier: Modifier = Modifier,
onUpdateClick: () -> Unit,
Copy link
Member

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 馃

Comment on lines +23 to 25
model: UiModel?,
viewModel: ThirdViewModel = hiltViewModel(),
navigator: (destination: BaseDestination) -> Unit,
Copy link
Member

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.

) {
ThirdScreenContent(data = model)
}

@Composable
fun ThirdScreenContent(data: UiModel?) {
fun ThirdScreenContent(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

must be private

Comment on lines +30 to 32
isResultOk: Boolean = false,
viewModel: HomeViewModel = hiltViewModel(),
navigator: (destination: BaseDestination) -> Unit,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

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

Successfully merging this pull request may close these issues.

Integrate the Twitter Jetpack Compose Rules Detekt plugin
6 participants