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

Upgrade to new spotless version for all samples #1443

Closed
wants to merge 5 commits into from

Conversation

riggaroo
Copy link
Collaborator

@riggaroo riggaroo commented Aug 12, 2024

Upgrade to version 1.3.1 of Ktlint, migrating to the standard rules where applicable. This change is the result of running the new rules - its large since it hasn't been run in a long time unfortunately.

Two rules disabled as per Compose conventions:
Composable function names, allow them to start with lowercase
Allowing PascalCase naming on constants

@riggaroo riggaroo marked this pull request as ready for review August 12, 2024 12:00
@riggaroo riggaroo requested a review from a team as a code owner August 12, 2024 12:00
@riggaroo riggaroo requested review from JolandaVerhoef and removed request for florina-muntenescu August 12, 2024 12:39
@@ -0,0 +1,9 @@
root = true

[*.{kt,kts}]
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about commenting why we're adding these exceptions? I'm assuming it's so that the different linters / checkers are in sync?

compileSdk =
libs.versions.compileSdk
.get()
.toInt()
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a bit overly optimized, are we sure we want this level of formatting?

border = BorderStroke(2.dp, borderColor)
modifier =
modifier
.padding(8.dp),
Copy link
Contributor

Choose a reason for hiding this comment

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

Again here, this doesn't look very readable, WDYT?

plugins {
alias(libs.plugins.gradle.versions)
alias(libs.plugins.version.catalog.update)
alias(libs.plugins.android.application) apply false
alias(libs.plugins.kotlin.android) apply false
alias(libs.plugins.kotlin.parcelize) apply false
alias(libs.plugins.compose) apply false
alias(libs.plugins.spotless)
Copy link
Contributor

Choose a reason for hiding this comment

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

One of the issues with Spotless is that it's not compatible with the Gradle configuration cache. This is why it should only be applied using an init script, rather than being part of the project build config.

@dturner
Copy link
Contributor

dturner commented Aug 12, 2024

Following discussion, I thought it'd be helpful to describe the process for identifying and disabling a ktlint rule which is run through spotless.

To disable a ktlint rule, you need to know 2 things:

  • the rule name
  • the ruleset which it appears in

The problem is that running gradlew spotlessCheck doesn't provide the names of the rules which are being violated (reported here, here and here). To find that out, you need to run ktlint directly (brew install ktlint).

I ran it on the JetLagged source using ktlint "app/src/**/*.kt" after rolling back some of the formatting changes in this PR. This gives you the names of any rule violations. In my case, one of the rules being violated was standard:multiline-expression-wrapping meaning it's in the standard ruleset and is named multiline-expression-wrapping.

To disable it, add an entry to .editorconfig using the following format ktlint_<ruleset>_<rulename> = disabled so in this case it's ktlint_standard_multiline-expression-wrapping = disabled.

BUT WAIT. Spotless is problematic because it doesn't work with configuration caching so you may find that it doesn't pick up the changes in .editorconfig (as reported here). To solve this use --no-configuration-cache and --no-daemon flags, so: ./gradlew spotlessCheck --no-daemon --no-configuration-cache.

Base automatically changed from jetsnack/m3 to compose-latest August 13, 2024 09:03
@riggaroo riggaroo closed this Aug 14, 2024
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.

3 participants