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.

Sign up for GitHub

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

Jump to bottom

D.marin/ktlint #1081

Merged
merged 6 commits into from
Oct 12, 2022
Merged

D.marin/ktlint #1081

merged 6 commits into from
Oct 12, 2022

Conversation

daniel-m-dd
Copy link
Contributor

@daniel-m-dd daniel-m-dd commented Oct 12, 2022 •
edited
Loading

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)

@daniel-m-dd daniel-m-dd marked this pull request as ready for review October 12, 2022 02:38
@daniel-m-dd daniel-m-dd requested a review from a team as a code owner October 12, 2022 02:38
@xgouchet xgouchet added the size-medium This PR is medium sized label Oct 12, 2022
xgouchet
xgouchet reviewed Oct 12, 2022
View reviewed changes
Copy link
Member

@xgouchet xgouchet left a comment

Choose a reason for hiding this comment

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

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
Copy link

codecov-commenter commented Oct 12, 2022 •
edited
Loading

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
Copy link
Contributor Author

daniel-m-dd commented Oct 12, 2022 •
edited
Loading

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
0xnm reviewed Oct 12, 2022
View reviewed changes
.gitlab-ci.yml
script:
- GRADLE_OPTS="-Xmx2560m" ./gradlew :ktlintCheckAll --stacktrace --no-daemon
- echo "Using Ktlint version $(ktlint --version)"
Copy link
Member

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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
0xnm reviewed Oct 12, 2022
View reviewed changes
build.gradle.kts
@@ -23,7 +23,6 @@ buildscript {
classpath(libs.androidToolsGradlePlugin)
classpath(libs.kotlinGradlePlugin)
classpath(libs.kotlinSPGradlePlugin)
classpath(libs.ktLintGradlePlugin)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?)

Copy link
Member

Choose a reason for hiding this comment

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

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 :)

@daniel-m-dd daniel-m-dd requested a review from xgouchet October 12, 2022 07:41
@xgouchet xgouchet added the size-large This PR is large sized label Oct 12, 2022
xgouchet
xgouchet reviewed Oct 12, 2022
View reviewed changes
CONTRIBUTING.md
@@ -48,9 +48,6 @@ The whole project is covered by a set of static analysis tools, linters and test
# launches the detekt static analysis for all modules
./gradlew detektAll

# launches the ktlint format check for all modules
Copy link
Member

Choose a reason for hiding this comment

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

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/**'

CONTRIBUTING.md Outdated
Comment on lines 184 to 190
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

Copy link
Member

Choose a reason for hiding this comment

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

Same here

0xnm
0xnm approved these changes Oct 12, 2022
View reviewed changes
dd-sdk-android-ktx/src/main/kotlin/com/datadog/android/ktx/coroutine/CoroutineExt.kt
@@ -21,6 +19,8 @@ import kotlinx.coroutines.async
import kotlinx.coroutines.launch
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.withContext
import kotlin.coroutines.CoroutineContext
Copy link
Member

Choose a reason for hiding this comment

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

strange that kotlin is places below kotlinx, but okay

@daniel-m-dd daniel-m-dd merged commit 8c0ac9c into develop Oct 12, 2022
@daniel-m-dd daniel-m-dd deleted the d.marin/ktlint branch October 12, 2022 23:58
@xgouchet xgouchet added this to the 1.15.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xgouchet xgouchet xgouchet approved these changes

@mariusc83 mariusc83 mariusc83 approved these changes

@0xnm 0xnm 0xnm approved these changes

Assignees
No one assigned
Labels
size-large This PR is large sized size-medium This PR is medium sized
Projects
None yet
Milestone
1.15.0
Development

Successfully merging this pull request may close these issues.
5 participants
@daniel-m-dd @codecov-commenter @xgouchet @mariusc83 @0xnm