ANDROID-11179 detekt implementation #386

jmanriquehiberus · 2024-09-18T15:36:47Z

🎟️ Jira ticket

🥅 What's the goal?

We need to implement detekt.

🚧 How do we do it?

mistica-android/detekt-baseline.xml
Some custom code smells added so detekt can spot them (taken from android-messenger).

mistica-android/build-tools/detekt/detekt.yml
Custom definition of detekt's behaviour.

mistica-android/build-tools/detekt.gradle
Gradle file, applied from the project's build.gradle in order to override gradle's detekt.

I also fixed 57 detekt issues, most of them are fixed in atomic commits in order to ease the review. Still, there are 39 issues to be addressed which are suppressed by now. I will create now some jira tickets to tackle them.

🧪 How can I test this?

Run the command ./gradlew detekt in order to run the task, modify detekt.yml and see that those new changes affect the output of the task.

jmanriquehiberus · 2024-09-18T15:38:15Z

Should we include detekt in any of our GAs?

github-actions · 2024-09-18T15:40:45Z

📱 New catalog for testing generated: Download

juangardi21

LGTM, just one suggestion 👍🏼

juangardi21 · 2024-09-19T07:04:59Z

build-tools/detekt.gradle

+    buildUponDefaultConfig = true
+    config = files("$projectDir/build-tools/detekt/detekt.yml")
+    input = files("$projectDir")
+}


You could exclude some folders like:

source.from(fileTree(rootProject.projectDir) { exclude("**/.gradle/") exclude("**/build/") exclude("**/tmp/**") })

True, just did it, thanks ☺️

github-actions · 2024-09-19T07:21:50Z

📱 New catalog for testing generated: Download

dpastor · 2024-09-19T14:01:50Z

build-tools/detekt.gradle

@@ -0,0 +1,12 @@
+
+detekt {


No files to adapt in mistica after adding detekt? Did you check this is executed with the ./gradlew check command? (which is the one executed on preintegration GA)

Yes, there are 96 issues to address after running detekt.

I am sorry but could not find any preintegration GA in mistica, should there be or should we include detekt in any GA? 🤔

It's the tests.yml file, the checks which are run on each PR commit, detekt should run as part of them.

Normally, when invoking "check" gradle target (executed on that GA), detekt is also executed with just applying the plugin, but please check it just to ensure it.

In order to add detekt we need to also fix the current issues repository has (or ignoring them someway to do that work afterwards), if it's too complicated, please check with the team how to proceed on next weekly or by chat (I'm on vacations next week).

Okay. By now, I have addressed 53 issues and still have 43 to be fixed (most of them related to long functions or too complex functions). I will address simpler issues alone and will keep potentially troublesome ones to be tackled along with the team.

…ing more autogenerated files

github-actions · 2024-09-20T09:04:58Z

📱 New catalog for testing generated: Download

github-actions · 2024-09-20T09:13:36Z

📱 New catalog for testing generated: Download

…sue after spotting unused "a" variable

github-actions · 2024-09-20T10:31:04Z

📱 New catalog for testing generated: Download

… order to fix detekt's issue

…naming ButtonSizeConfig data class or ButtonSizes.kt

github-actions · 2024-09-20T12:24:17Z

📱 New catalog for testing generated: Download

jeprubio

👍

jeprubio · 2024-09-24T09:11:45Z

build.gradle

+    tasks.withType(io.gitlab.arturbosch.detekt.Detekt).all {
+        exclude("build/")
+        exclude("**/build/**")
+        exclude("**/resources/**")
+        exclude("**/tmp/**")
+    }


these are also in detekt.gradle ¿are they needed in both places? ¿Perhaps if this is here the ones from detekt.gradle are not needed?

True, just committed a code cleanup. Thanks!

jeprubio · 2024-09-24T09:16:08Z

detekt-baseline.xml

+<?xml version="1.0" ?>
+<SmellBaseline>
+  <ManuallySuppressedIssues></ManuallySuppressedIssues>
+  <CurrentIssues>
+    <ID>WildcardImport:protect_android_task.gradle.kts$import java.io.*</ID>
+    <ID>WildcardImport:protect_android_task.gradle.kts$import java.lang.*</ID>
+  </CurrentIssues>
+</SmellBaseline>


I think we could either fix these imports or use @Supress* in that files in order not to need this baseline. I fear we'll fix this at some point and forget to remove the baseline.

I did not have to fix any issues here, it was my bad. Thanks for spotting it.

Thinking it twice, it makes no sense to have an empty detekt-baseline.xml file for now, so I am deleting it.

github-actions · 2024-09-24T10:04:40Z

📱 New catalog for testing generated: Download

github-actions · 2024-09-24T10:15:59Z

📱 New catalog for testing generated: Download

juangardi21

Awesome

…nrique/ANDROID-11179-enable-detekt-in-mistica-android # Conflicts: # README.md

github-actions · 2024-09-24T13:07:39Z

📱 New catalog for testing generated: Download

github-actions · 2024-09-24T13:16:00Z

📱 New catalog for testing generated: Download

nimeacuerdo · 2024-09-24T13:45:58Z

Breaking changes that require a new major release? if so I would expect updates in https://github.com/Telefonica/mistica-android/blob/jmanrique/ANDROID-11179-enable-detekt-in-mistica-android/UPGRADING.md

Any ticket created to tackle pending issues?

jmanriquehiberus · 2024-09-24T14:23:46Z

Breaking changes that require a new major release? if so I would expect updates in https://github.com/Telefonica/mistica-android/blob/jmanrique/ANDROID-11179-enable-detekt-in-mistica-android/UPGRADING.md

Any ticket created to tackle pending issues?

I have just created these 3 tickets to tackle pending issues.

I have a doubt about this being a major release. I would say this is a minor one, since there are no breaking changes (I think), it is just adding new functionality (detekt) which is backward compatible. What am I missing?

nimeacuerdo · 2024-09-24T14:46:02Z

Breaking changes that require a new major release? if so I would expect updates in https://github.com/Telefonica/mistica-android/blob/jmanrique/ANDROID-11179-enable-detekt-in-mistica-android/UPGRADING.md
Any ticket created to tackle pending issues?

I have just created these 3 tickets to tackle pending issues.

https://jira.tid.es/browse/ANDROID-15230

https://jira.tid.es/browse/ANDROID-15231

https://jira.tid.es/browse/ANDROID-15232

I have a doubt about this being a major release. I would say this is a minor one, since there are no breaking changes (I think), it is just adding new functionality (detekt) which is backward compatible. What am I missing?

Thanks for creating those tickets!

what about this https://github.com/Telefonica/mistica-android/pull/386/files#diff-16c7f6781a03c2127c9cfd4b50a25cd8ad71e464e4848fccbcc9a823e44f73bdL222-R222?

jmanriquehiberus · 2024-09-24T14:49:13Z

Breaking changes that require a new major release? if so I would expect updates in https://github.com/Telefonica/mistica-android/blob/jmanrique/ANDROID-11179-enable-detekt-in-mistica-android/UPGRADING.md
Any ticket created to tackle pending issues?

I have just created these 3 tickets to tackle pending issues.

https://jira.tid.es/browse/ANDROID-15230

https://jira.tid.es/browse/ANDROID-15231

https://jira.tid.es/browse/ANDROID-15232

I have a doubt about this being a major release. I would say this is a minor one, since there are no breaking changes (I think), it is just adding new functionality (detekt) which is backward compatible. What am I missing?

Thanks for creating those tickets!

what about this https://github.com/Telefonica/mistica-android/pull/386/files#diff-16c7f6781a03c2127c9cfd4b50a25cd8ad71e464e4848fccbcc9a823e44f73bdL222-R222?

Sorry, thanks!! I did not know that was a breaking change, I am adding it to https://github.com/Telefonica/mistica-android/blob/jmanrique/ANDROID-11179-enable-detekt-in-mistica-android/UPGRADING.md as you mentioned.

jeslat · 2024-09-24T14:52:22Z

catalog/src/main/java/com/telefonica/mistica/catalog/ui/compose/components/Buttons.kt

@@ -50,6 +50,7 @@ fun Buttons() {
 }

 @Composable
+@Suppress("LongMethod")


I would include an ignore to not take into account the composables as we do in platform:

LongMethod: ignoreAnnotated: 'Composable'

In general, most of the composables will throw the long method error

Great, I have just included the ignore you mentioned and there are only 2 LongMethod flagged non composable functions. Thank you!!!

jeslat · 2024-09-24T14:55:59Z

Breaking changes that require a new major release? if so I would expect updates in https://github.com/Telefonica/mistica-android/blob/jmanrique/ANDROID-11179-enable-detekt-in-mistica-android/UPGRADING.md
Any ticket created to tackle pending issues?

I have just created these 3 tickets to tackle pending issues.

https://jira.tid.es/browse/ANDROID-15230

https://jira.tid.es/browse/ANDROID-15231

https://jira.tid.es/browse/ANDROID-15232

I have a doubt about this being a major release. I would say this is a minor one, since there are no breaking changes (I think), it is just adding new functionality (detekt) which is backward compatible. What am I missing?

Thanks for creating those tickets!
what about this https://github.com/Telefonica/mistica-android/pull/386/files#diff-16c7f6781a03c2127c9cfd4b50a25cd8ad71e464e4848fccbcc9a823e44f73bdL222-R222?

Sorry, thanks!! I did not know that was a breaking change, I am adding it to https://github.com/Telefonica/mistica-android/blob/jmanrique/ANDROID-11179-enable-detekt-in-mistica-android/UPGRADING.md as you mentioned.

@jmanriquehiberus as it's explained in that document, now the breaking changes are explained in the release notes of the new version, in this case 15.0.0. You have an example here https://github.com/Telefonica/mistica-android/releases/tag/14.0.0

jmanriquehiberus · 2024-09-24T14:57:33Z

Breaking changes that require a new major release? if so I would expect updates in https://github.com/Telefonica/mistica-android/blob/jmanrique/ANDROID-11179-enable-detekt-in-mistica-android/UPGRADING.md
Any ticket created to tackle pending issues?

I have just created these 3 tickets to tackle pending issues.

https://jira.tid.es/browse/ANDROID-15230

https://jira.tid.es/browse/ANDROID-15231

https://jira.tid.es/browse/ANDROID-15232

I have a doubt about this being a major release. I would say this is a minor one, since there are no breaking changes (I think), it is just adding new functionality (detekt) which is backward compatible. What am I missing?

Thanks for creating those tickets!
what about this https://github.com/Telefonica/mistica-android/pull/386/files#diff-16c7f6781a03c2127c9cfd4b50a25cd8ad71e464e4848fccbcc9a823e44f73bdL222-R222?

Sorry, thanks!! I did not know that was a breaking change, I am adding it to https://github.com/Telefonica/mistica-android/blob/jmanrique/ANDROID-11179-enable-detekt-in-mistica-android/UPGRADING.md as you mentioned.

@jmanriquehiberus as it's explained in that document, now the breaking changes are explained in the release notes of the new version, in this case 15.0.0. You have an example here https://github.com/Telefonica/mistica-android/releases/tag/14.0.0

Thank you!

jeslat

Thanks, good job!

github-actions · 2024-09-24T15:15:52Z

📱 New catalog for testing generated: Download

jmanriquehiberus added 2 commits September 18, 2024 12:28

ANDROID-11179 detekt implementation

23291e2

ANDROID-11179 using detekt's last version

fd59062

jmanriquehiberus requested review from a team, dpastor and juangardi21 and removed request for a team September 18, 2024 15:38

juangardi21 approved these changes Sep 19, 2024

View reviewed changes

ANDROID-11179 excluding files beneath .gradle, build & tmp

6baac3a

jmanriquehiberus self-assigned this Sep 19, 2024

dpastor reviewed Sep 19, 2024

View reviewed changes

jmanriquehiberus requested a review from dpastor September 19, 2024 15:42

ANDROID-11179 deprecated input replaced with basePath, and now exclud…

60f8f91

…ing more autogenerated files

ANDROID-11179 non deprecated config setting

6c144d6

jmanriquehiberus added 5 commits September 20, 2024 12:10

ANDROID-11179 interface name capitalisation to meet detekt requirements

fe5535a

ANDROID-11179 for loop replaced with repeat in order to fix detekt is…

01bc970

…sue after spotting unused "a" variable

ANDROID-11179 unused imports removed

5d18440

ANDROID-11179 too long line detekt issue fixed

e5ff9b8

ANDROID-11179 multiple variable declaration detekt issue fixed

b26150e

jmanriquehiberus added 3 commits September 20, 2024 14:13

ANDROID-11179 calling error function instead of throwing exception in…

75c1a6e

… order to fix detekt's issue

ANDROID-11179 putting composable functions first in order to avoid re…

46d4262

…naming ButtonSizeConfig data class or ButtonSizes.kt

ANDROID-11179 empty constructor removed

4edc4f2

jmanriquehiberus added 2 commits September 23, 2024 13:24

ANDROID-11179 detekt issues fixed LongMethod, CyclomaticComplexMethod

c9afd31

ANDROID-11179 detekt issues fixed SwallowedException

590835e

jmanriquehiberus requested review from jeprubio and removed request for a team September 24, 2024 08:48

jeprubio approved these changes Sep 24, 2024

View reviewed changes

ANDROID-11179 code cleanup

0e9f6c7

ANDROID-11179 detekt-baseline.xml removed

bada90c

ANDROID-11179 error prone icon replaced with stable one

ee638a9

juangardi21 approved these changes Sep 24, 2024

View reviewed changes

Merge branch 'main' of github.com:Telefonica/mistica-android into jma…

e93dd76

…nrique/ANDROID-11179-enable-detekt-in-mistica-android # Conflicts: # README.md

ANDROID-11179 link to the library updated

3ff5363

nimeacuerdo requested review from jeslat and removed request for dpastor September 24, 2024 13:40

jeslat reviewed Sep 24, 2024

View reviewed changes

ANDROID-11179 LongMethod issue ignored on Composable annotated functions

8305ea3

jmanriquehiberus requested review from dpastor and jeslat September 24, 2024 15:12

jeslat approved these changes Sep 24, 2024

View reviewed changes

jmanriquehiberus removed the request for review from dpastor September 24, 2024 15:57

jmanriquehiberus merged commit 4d44396 into main Sep 24, 2024
5 checks passed

jmanriquehiberus deleted the jmanrique/ANDROID-11179-enable-detekt-in-mistica-android branch September 24, 2024 15:57

ANDROID-11179 detekt implementation #386

ANDROID-11179 detekt implementation #386

Conversation

jmanriquehiberus commented Sep 18, 2024 • edited Loading

🎟️ Jira ticket

🥅 What's the goal?

🚧 How do we do it?

🧪 How can I test this?

jmanriquehiberus commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

juangardi21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 20, 2024

github-actions bot commented Sep 20, 2024

github-actions bot commented Sep 20, 2024

github-actions bot commented Sep 20, 2024

jeprubio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 24, 2024

github-actions bot commented Sep 24, 2024

juangardi21 left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 24, 2024

github-actions bot commented Sep 24, 2024

nimeacuerdo commented Sep 24, 2024 • edited Loading

jmanriquehiberus commented Sep 24, 2024

nimeacuerdo commented Sep 24, 2024 • edited Loading

jmanriquehiberus commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeslat commented Sep 24, 2024

jmanriquehiberus commented Sep 24, 2024

jeslat left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 24, 2024

jmanriquehiberus commented Sep 18, 2024 •

edited

Loading

nimeacuerdo commented Sep 24, 2024 •

edited

Loading

nimeacuerdo commented Sep 24, 2024 •

edited

Loading