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

Update Detekt to 1.23.4 #384

Closed
wants to merge 8 commits into from
Closed

Conversation

AzimMuradov
Copy link
Contributor

Should fix issues in #374 automatic PR

@AzimMuradov AzimMuradov marked this pull request as ready for review December 23, 2023 14:51
@AzimMuradov
Copy link
Contributor Author

AzimMuradov commented Dec 23, 2023

Changes:

I hope this change would make the build script more readable and generally nice to config.

  • Set up indentation level to 2 spaces

This style guide is already present in the project, so I decided to explicitly describe it in the .editorconfig and detekt.yml. The first one would help an IDE to format the code properly, and the second one will ensure that this style guide would be followed.

@AzimMuradov
Copy link
Contributor Author

AzimMuradov commented Dec 23, 2023

BTW, why does the build script use spotless and detekt at the same time? I think the detekt would be more than enough.

@AzimMuradov
Copy link
Contributor Author

AzimMuradov commented Dec 23, 2023

Also, I could filter out detekt tasks on ...Test source sets if it's needed.

It's as simple as changing

tasks {

    // ...

    afterEvaluate {
        check {
            dependsOn(withType<Detekt>())
        }
    }
}

to

tasks {

    // ...

    afterEvaluate {
        check {
            dependsOn(withType<Detekt>().filterNot { it.name.contains("test", ignoreCase = true) })

            // or even:
            // dependsOn(withType<Detekt>().filter { "Main" in it.name })
        }
    }
}

@oshai
Copy link
Owner

oshai commented Dec 25, 2023

BTW, why does the build script use spotless and detekt at the same time? I think the detekt would be more than enough.

If I remember correctly we started with detekt and now use spotless (don't remember why I changed it...). So I am not sure we need detekt at all.

@AzimMuradov
Copy link
Contributor Author

BTW, why does the build script use spotless and detekt at the same time? I think the detekt would be more than enough.

If I remember correctly we started with detekt and now use spotless (don't remember why I changed it...). So I am not sure we need detekt at all.

As far as I know, detekt is a full blown static analysis tool while spotless is more like a formatter/code style checker.

If you want, I can remove detekt from the config. If so, should I just rename this pr, or should I close it, and open the new one?

@AzimMuradov
Copy link
Contributor Author

AzimMuradov commented Dec 25, 2023

The reason I created this PR is because old detekt blocks the addition of the wasmJs target. If you don't currently want to remove detekt we can think about that later and for now I'll just update it's version.

@oshai
Copy link
Owner

oshai commented Jan 7, 2024

Let's stay only with spotless and remove detekt.

@AzimMuradov AzimMuradov mentioned this pull request Jan 8, 2024
@AzimMuradov AzimMuradov closed this Jan 8, 2024
@AzimMuradov AzimMuradov deleted the update-detekt branch January 11, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants