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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Detekt in the project #216

Merged
merged 14 commits into from
Jan 28, 2024
Merged

Adding Detekt in the project #216

merged 14 commits into from
Jan 28, 2024

Conversation

theolm
Copy link
Contributor

@theolm theolm commented Jan 23, 2024

Description:

This pull request aims to enhance the quality of our source code by replacing the ktlint code formatter with Detekt. Detekt is a powerful static analysis tool for Kotlin, providing comprehensive analyses to improve code readability and maintainability. Here are some of the benefits of using this tool:

  1. Comprehensive Static Analysis:

    • Detekt goes beyond basic formatting, offering more extensive static analyses to identify code patterns, anti-patterns, and other issues that can impact code quality.
  2. Flexible Configuration:

    • Detekt allows flexible configuration, adapting to the specific needs of our project. I already added all the ktlint rules and Compose rules so we don't need to rely on other linters.
  3. Integration with Android Studio:

    • Detekt provides direct integration with Android Studio via a plugin, enhancing the development experience for our Kotlin-based Android projects. When setting up the plugin, please don't forget to use our baseline and config, otherwise it will not work properly.
  4. Baseline for Existing Errors:

    • A baseline has been added to prevent detection of existing errors, allowing for a smoother transition. This ensures that the current codebase's issues are not flagged immediately.

Other changes

Workflow Updates:

  • Existing workflows have been updated to incorporate the changes related to Detekt integration, ensuring a seamless integration into our development processes.

Future work

In a subsequent PR, the baseline will be removed, and existing errors will be addressed. This phased approach allows us to adopt Detekt gradually without overwhelming the team with immediate corrections.

How to Test:

  1. Ensure the environment is correctly configured after merging this PR.
  2. Run ./gradlew detekt to ensure Detekt's static analyses are executed successfully.

More info:

Here is a report for all the issues found by the tool.

report.zip

Please let me know what you guys think.

@theolm theolm marked this pull request as ready for review January 23, 2024 22:15
@AntsyLich
Copy link
Member

Is detekt being applied to all modules?

@theolm
Copy link
Contributor Author

theolm commented Jan 24, 2024

The plugin is applied only to the app module, but the configuration is set to check files across the entire project (all the modules). I wrote a pre-compiled plugin with the detekt configuration. buildSrc/src/main/kotlin/detekt.gradle.kts

private val projectSource = file("$rootDir")
private val configFile = files("$rootDir/config/detekt/detekt.yml")
private val baselineFile = file("$rootDir/config/detekt/baseline.xml")
private val kotlinFiles = "**/*.kt"
private val resourceFiles = "**/resources/**"
private val buildFiles = "**/build/**"
private val generatedFiles = "**/generated/**"
private val scriptsFiles = "**/*.kts"

Basically, the tool will start in the projectSource and recursively look for files that match **/*.kt.

include(kotlinFiles)
exclude(resourceFiles, buildFiles, generatedFiles, scriptsFiles)

@AntsyLich
Copy link
Member

Can you like make a convention plugin and apply it to per module? Don't really want to run the whole task with :app

@theolm
Copy link
Contributor Author

theolm commented Jan 24, 2024

@AntsyLich Sure, I can add it pre-module.
I just don't see the reason for using a convention plugin. do you have any strong feelings about it? I would rather modify my Kotlin script and apply it in the subprojects (like it was with the ktlint).

I just realized that the pre-compiled plugin that I wrote is, in fact, a convention plugin. I was under the idea that the convention plugin was the old plugins written in groovy.

@AntsyLich
Copy link
Member

I plan to migrate everything to convention plugin

@theolm
Copy link
Contributor Author

theolm commented Jan 24, 2024

@AntsyLich Let me know what you think.

buildSrc/build.gradle.kts Show resolved Hide resolved
.github/workflows/build_pull_request.yml Outdated Show resolved Hide resolved
gradle/compose.versions.toml Show resolved Hide resolved
@theolm
Copy link
Contributor Author

theolm commented Jan 28, 2024

@AntsyLich Please ping me before merging this PR.
Several PRs have been merged since I opened this one, so I would like to update the baseline before the merge. This will avoid problems.

@AntsyLich
Copy link
Member

I may have forgotten to ask you to rebase. Do that and ping me I'll merge it afterwards

@theolm
Copy link
Contributor Author

theolm commented Jan 28, 2024

@AntsyLich Done.

@AntsyLich
Copy link
Member

AntsyLich commented Jan 28, 2024

Optionally, it is possible to slim down the configuration file to only the changes from the default configuration, by applying the buildUponDefaultConfig option

We don't need that huge ass yml file do we? Also use https://github.com/mrmans0n/compose-rules (https://mrmans0n.github.io/compose-rules/detekt)

@theolm
Copy link
Contributor Author

theolm commented Jan 28, 2024

@AntsyLich
Ok, I'll replace the Compose rule set.
about the config file, are you sure you to slim down the file? I found very useful to have all the rules listed.

@AntsyLich
Copy link
Member

it's fine to slim down the config file. chances are we will just keep following the default

@theolm
Copy link
Contributor Author

theolm commented Jan 28, 2024

@AntsyLich Done.

@AntsyLich
Copy link
Member

According to https://mrmans0n.github.io/compose-rules/detekt/ you need to enable some rules 🤔 one of them also seem to supersede FunctionNaming if I'm not wrong

@theolm
Copy link
Contributor Author

theolm commented Jan 28, 2024

@AntsyLich The plugin rules are enabled by default after applying the plugin. I trimmed down the config files removing all the rules with default values (as you asked).

Here is the generated config file after applying the plugin.
Screenshot 2024-01-28 at 6 24 16 PM

You can also see that the baseline has several compose-rules on it.

@AntsyLich AntsyLich merged commit cc09230 into mihonapp:main Jan 28, 2024
1 check passed
@AntsyLich
Copy link
Member

Any reason why you didn't use detekt v1.23.4?

kaiserbh pushed a commit to kaiserbh/mihon that referenced this pull request Feb 18, 2024
* Removing ktlint

* Removing compose lint

* Adding initial Detekt config

* Setting up detekt config

* Adding detekt baseline

* Fixing workflows

* Moving to a module based solution

* Adding new line

* Adding new line

* Updating baseline

* Addressing PR suggestions

* Regenerating baseline.xml

* Cleanup

---------

Co-authored-by: AntsyLich <59261191+AntsyLich@users.noreply.github.com>
(cherry picked from commit cc09230)

# Conflicts:
#	.github/workflows/build_pull_request.yml
#	.github/workflows/build_push.yml
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants