code4romania / teacher-workout-android

Teacher Workout Android app
Mozilla Public License 2.0
5 stars 17 forks source link

Add detekt #22

Closed AlexandraDamaschin closed 3 years ago

AlexandraDamaschin commented 3 years ago

What does it fix?

Add detekt, ktlinFormat & lint

Closes #XXX

How has it been tested?

AlexandraDamaschin commented 3 years ago

@lukstbit here I'm trying to add detekt, lint and ktlintFormat to the project. I've made some changes and moved the colors to the app module, but, by doing that now I'm not able to access them from the features module. Maybe you know more about this and can help me with that?

lukstbit commented 3 years ago

@AlexandraDamaschin Did you encounter errors related to those resources when implementing the code quality plugins? My thinking was to have the commons/ui module as a dependency for feature modules and there to keep all the ui related stuff(like colors and themes, or common composables). The dependency tree will then be :

app module depends on feature module/ feature module dependens on commons/ui module(and if needed make the common/ui module an api configuration in the build file so the resources are also available in the app module)

AlexandraDamaschin commented 3 years ago

@AlexandraDamaschin Did you encounter errors related to those resources when implementing the code quality plugins? My thinking was to have the commons/ui module as a dependency for feature modules and there to keep all the ui related stuff(like colors and themes, or common composables). The dependency tree will then be :

app module depends on feature module/ feature module dependens on commons/ui module(and if needed make the common/ui module an api configuration in the build file so the resources are also available in the app module)

Yeah, When running lint -> lint would say that the colors/themes aren't used in the code and should be deleted -> so lint failed. Moving the colors/themes to the app module and use them from there was my solution for this as the feature module depends on the app module. I've found solutions to use the resources from the app module, but only in XML, there wasn't any information about how to do using compose.

lukstbit commented 3 years ago

I ran your code from this PR up until the last commit(the one moving the colors) and run lint. I only get 3 issues:

AllowBackup: AllowBackup/FullBackupContent Problems ObsoleteSdkInt: Obsolete SDK_INT Version Check ConvertToWebp: Convert to WebP

No other issues. Could you revert the last commit and only use the code to fix the above issues and then merge the PR? At the moment the checks fail anyway.

AlexandraDamaschin commented 3 years ago

@lukstbit could you take a look and if everything is ok we can merge it? Also, pretty strange that when I run ./gradlew detekt locally it showed me the errors with the colors, but here it didn't. Any ideas why could that happen? Just as a nice to know for me.

lukstbit commented 3 years ago

LGTM :+1:

Also, pretty strange that when I run ./gradlew detekt locally it showed me the errors with the colors, but here it didn't. Any ideas why could that happen? Just as a nice to know for me.

I couldn't say why. There is no reason to see those colors as unused because they were used in the themes in the same module.