D.marin/ktlint by daniel-m-dd · Pull Request #1081 · DataDog/dd-sdk-android

daniel-m-dd · 2022-10-12T02:32:22Z

What does this PR do?

Update Ktlint to 0.45.1 and use a dedicated runner image

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

xgouchet

Nice job. One quick thought, we might want to remove the ktLint Gradle plugin which mentions a different version of ktLint and might return different reports when ran locally

codecov-commenter · 2022-10-12T07:25:13Z

Codecov Report

Merging #1081 (a36fdcc) into develop (7df131c) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #1081      +/-   ##
===========================================
- Coverage    83.19%   83.16%   -0.03%     
===========================================
  Files          272      272              
  Lines         9279     9279              
  Branches      1494     1494              
===========================================
- Hits          7719     7716       -3     
- Misses        1143     1145       +2     
- Partials       417      418       +1

Impacted Files	Coverage Δ
...datadog/android/coil/DatadogCoilRequestListener.kt	`100.00% <ø> (ø)`
...com/datadog/android/compose/InteractionTracking.kt	`73.55% <ø> (ø)`
...in/com/datadog/android/glide/DatadogGlideModule.kt	`94.74% <ø> (ø)`
.../com/datadog/android/ktx/coroutine/CoroutineExt.kt	`75.86% <ø> (ø)`
...kotlin/com/datadog/android/DatadogEventListener.kt	`98.51% <ø> (ø)`
...n/kotlin/com/datadog/android/DatadogInterceptor.kt	`93.67% <ø> (ø)`
...atadog/android/core/configuration/Configuration.kt	`98.70% <ø> (ø)`
...n/com/datadog/android/core/internal/CoreFeature.kt	`91.60% <ø> (ø)`
...tadog/android/core/internal/net/CurlInterceptor.kt	`83.33% <ø> (ø)`
.../android/core/internal/net/DataOkHttpUploaderV2.kt	`97.62% <ø> (ø)`
... and 17 more

daniel-m-dd · 2022-10-12T07:31:11Z

Nice job. One quick thought, we might want to remove the ktLint Gradle plugin which mentions a different version of ktLint and might return different reports when ran locally

Indeed, updated

0xnm · 2022-10-12T07:31:21Z

+  image: "registry.ddbuild.io/mobile/images/ktlint:v0.1.0"
  script:
-    - GRADLE_OPTS="-Xmx2560m" ./gradlew :ktlintCheckAll --stacktrace --no-daemon
+    - echo "Using Ktlint version $(ktlint --version)"


it also means we have to keep local ktlint version in sync with the version used in the image (because the rules may be different)?

Yes, exactly.
In a second step, we might find a way to ensure this condition or make it easier to upgrade/be in sync. Any ideas?

I guess the only way is to just build a new docker image with a different tag (or have a fat docker image with multiple ktlint versions embedded).

0xnm · 2022-10-12T07:37:53Z

        classpath(libs.androidToolsGradlePlugin)
        classpath(libs.kotlinGradlePlugin)
        classpath(libs.kotlinSPGradlePlugin)
-        classpath(libs.ktLintGradlePlugin)


I'm wondering what the flow will be. Before I was running ktlintFormat before pushing to git - at which step formatting will be done now? Also sometimes ktlintFormat fails to do the job, like in case of too long lines - in that case some manual changes were needed.

You ca still run ktlint -F "**/*.kt" "**/*.kts" '!**/build/generated/**' '!**/build/kspCaches/**' or make it a pre-commit hook. Will behave the same.
However I guess I should update the doc with the current required ktlint version (0.45.1)

but in this case I need to install ktlint CLI on my machine manually, do I get it right? Or you are using the Dockerimage for that which has a project folder mounted to it?

I personally use the local ktlint CLI on my machine (the gradle plugin was just already a wrapper around the CLI on your machine no?)

No, Gradle plugin is not a wrapper for the existing ktlint, I don't have ktlint installed on my machine. But I see the proposed approach now :)

xgouchet · 2022-10-12T08:14:23Z

 # launches the detekt static analysis for all modules
 ./gradlew detektAll

-# launches the ktlint format check for all modules


Can we mention the ktlint cli here instead to make sure contributors know that this is a check they should use:

ktlint -F "**/*.kt" "**/*.kts" '!**/build/generated/**' '!**/build/kspCaches/**'

xgouchet · 2022-10-12T08:14:38Z

-There is also a gradle task that you can use to automatically format the code following the
-required styling rules:
-
-```console
-
-./gradlew :dd-sdk-android:ktlintFormat
-


0xnm · 2022-10-12T12:31:00Z

 import kotlinx.coroutines.launch
 import kotlinx.coroutines.runBlocking
 import kotlinx.coroutines.withContext
+import kotlin.coroutines.CoroutineContext


strange that kotlin is places below kotlinx, but okay

Update ktlint task to use a dedicated image

01e5722

daniel-m-dd force-pushed the d.marin/ktlint branch from 233275d to 01e5722 Compare October 12, 2022 02:33

Update ktlint to 0.45.1

581817c

daniel-m-dd marked this pull request as ready for review October 12, 2022 02:38

daniel-m-dd requested a review from a team as a code owner October 12, 2022 02:38

xgouchet added the size-medium This PR is medium sized label Oct 12, 2022

xgouchet reviewed Oct 12, 2022

View reviewed changes

xgouchet approved these changes Oct 12, 2022

View reviewed changes

0xnm reviewed Oct 12, 2022

View reviewed changes

daniel-m-dd force-pushed the d.marin/ktlint branch from 4915e92 to 2716d12 Compare October 12, 2022 07:34

Remove ktlint gradle plugin

3fa28c1

daniel-m-dd force-pushed the d.marin/ktlint branch from 2716d12 to 3fa28c1 Compare October 12, 2022 07:36

0xnm reviewed Oct 12, 2022

View reviewed changes

Add Ktlnt version to CONTRIBUTING

ae74c71

daniel-m-dd requested a review from xgouchet October 12, 2022 07:41

Rollback formatting on CONTRIBUTING.md

8bb2803

xgouchet added the size-large This PR is large sized label Oct 12, 2022

xgouchet reviewed Oct 12, 2022

View reviewed changes

Update CONTRIBUTING with ktlint format

a36fdcc

mariusc83 approved these changes Oct 12, 2022

View reviewed changes

xgouchet approved these changes Oct 12, 2022

View reviewed changes

0xnm approved these changes Oct 12, 2022

View reviewed changes

daniel-m-dd merged commit 8c0ac9c into develop Oct 12, 2022

daniel-m-dd deleted the d.marin/ktlint branch October 12, 2022 23:58

xgouchet added this to the 1.15.0 milestone Dec 13, 2023

Uh oh!

Conversation

daniel-m-dd commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

xgouchet left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

daniel-m-dd commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

daniel-m-dd commented Oct 12, 2022 •

edited

Loading

codecov-commenter commented Oct 12, 2022 •

edited

Loading

daniel-m-dd commented Oct 12, 2022 •

edited

Loading