facebook / ktfmt

A program that reformats Kotlin source code to comply with the common community standard for Kotlin code conventions.
https://facebook.github.io/ktfmt/
Apache License 2.0
857 stars 66 forks source link

Android Studio / IDEA plugin yields different results than running ktfmt #444

Open gino-m opened 3 months ago

gino-m commented 3 months ago

Originally posted in https://github.com/cortinico/ktfmt-gradle/issues/264

🐛 Describe the bug

Formatting with the Android Studio ktfmt plugin produces a different result than ktfmtFormat via Gradle and https://facebook.github.io/ktfmt/.

⚠️ Current behavior

./gradlew ktfmtFormat target (or trigger via IDE) provided by Gradle plugin com.ncorti.ktfmt.gradle version 0.17.0:

@Retention(AnnotationRetention.RUNTIME) annotation class SharedViewModel

"Format code" using Android Studio ktfmt plugin 1.1.0.47:

@Retention(AnnotationRetention.RUNTIME)
annotation class SharedViewModel

More deltas can be found in https://github.com/google/ground-android/pull/2412/files. Base files are formatted via the Gradle plugin, modified files with AS plugin.

✅ Expected behavior

Both formatters produce the same result.

@cortinico @cgrushko @sufyanAbbasi @shobhitagarwal1612 FYI

hick209 commented 3 months ago

@greyhairredbear do you know if this is related to #420?

hick209 commented 3 months ago

I tried to format you code here and it fails due to the @interface there, as that's not valid Kotlin.

Could you revisit and share a piece of code where we could repro the issue?

greyhairredbear commented 3 months ago

@greyhairredbear do you know if this is related to #420?

Unfortunately, not really. My original thought was that this might be because of different ktfmt versions, but according to the original post in the gradle plugin, this doesn't seem to be the case.

~I can try if #420 produces the expected results.~

UPDATE: @hick209 of course, you are correct, the @interface is not valid Kotlin... @gino-m are you sure this is a .kt file you are referencing here?

PS: @gino-m I suppose it would be helpful to also include the versions used in this post.

greyhairredbear commented 3 months ago

Just FYI: When using #420, the code stays unchanged (regardless of the formatting) due to it not being valid Kotlin. After removing the @interface-portion, the output is the following:

/**
 * Annotates view models to indicate that a single instance should be shared by all fragments in an
 * activity. Only one instance of such view models will be created per activity by {@link
 * ViewModelFactory#get(Fragment, Class)}.
 */
@Retention(RetentionPolicy.RUNTIME) public SharedViewModel {}
gino-m commented 3 months ago

@hick209 So sorry, was moving too fast. That was Java - added a Kotlin example .

@greyhairredbear The formatting in the above snippet was produced by Android Studio ktfmt plugin 1.1.0.47, the Gradle plugin com.ncorti.ktfmt.gradle version 0.17.0. Added these to the updated issue description and shared additional deltas in https://github.com/google/ground-android/pull/2412/files.

blipinsk commented 2 months ago

@gino-m this might be related: https://github.com/cortinico/ktfmt-gradle/issues/277

greyhairredbear commented 2 months ago

FYI: Just tried the new version of the plugin released last week and the code you mentioned

@Retention(AnnotationRetention.RUNTIME)
annotation class SharedViewModel

gets reformatted to the code produced by the gradle plugin

@Retention(AnnotationRetention.RUNTIME) annotation class SharedViewModel

Seems to me this is not an issue anymore - probably has been fixed with #420. @gino-m could you verify that also is the case with the code you've mentioned, so we can close this?

gino-m commented 2 months ago

Confirmed, I upgraded to 1.1.0.49 and the outputs for this particular use case now match.

There are still some differences, which I've tried to capture here: https://github.com/google/ground-android/pull/2456/commits

The first run of the latest AS plugin across our codebase introduced many changes, most of which are not modified by running ktfmt via Gradle again. Subsequent commits demonstrate formatting which is changes by both entry points on each run, namely: https://github.com/google/ground-android/pull/2456/commits/b9736a1963342e0977f36c98f000e1e672a1a4fb

blipinsk commented 2 months ago

@gino-m did you also update the ktfmt-gradle to the newer version (0.18.0)? I suspect the intellij plugin is working correctly but your codebase was formatted incorrectly with the older Gradle plugin

gino-m commented 2 months ago

Good catch. I upgraded ktfmt-gradle to 0.18.0 and reformatted (https://github.com/google/ground-android/pull/2456/commits/9dc9da5b33a56f44daf19048ab7ceb57e1e3905e).

Reformatting with AS ktfmt 1.1.0.49 still yields several diffs (https://github.com/google/ground-android/pull/2456/commits/e6a49740a0bbba721bd2f3dd0e0a473c11ed3626), but subsequent runs of ktfmt-gradle only modify three files (https://github.com/google/ground-android/pull/2456/commits/ea607277c5420397ec68d60d7217b17e12615e66). Two of these changes get undone when running the AS plugin again (https://github.com/google/ground-android/pull/2456/commits/ebe230ec89038b5920dbc4d97a07284cd7704f65).

At this point I would expect the diffs, if any, to be symmetrical, but running ktfmt-gradle again only modifies one file (https://github.com/google/ground-android/pull/2456/commits/81371bea3efcea3109b94076124afb1ae4347ecc). After that, subsequent runs fight over a single delta (https://github.com/google/ground-android/pull/2456/commits/e26f8d91605478dc22c2a7f7757efcb2635e0a2c).

Edit: Pasting screenshot here for convenience. Before = ktfmt-gradle 0.180, After = AS ktfmt 1.1.0.49:

image

If that delta is resolved the formatting should be stable across both plugins after each is run several times.

greyhairredbear commented 2 months ago

What is the output of the CLI? I ran ktfmt --google-style Config.kt locally and got the second output - so it seems the plugin is behaving as expected.