0.52 and trailing commas behavior on Kotlinlang formatting

[JavierSegoviaCordoba]

Discussed in #510

^{Originally posted by JavierSegoviaCordoba August 20, 2024}
After upgrading I am seeing trailing commas being removed and I think it is not only worse because it is less flexible in terms of how you can format the code (decide to have multiline or not by adding a trailing comma), but objectively it can modify the code in a way it transforms something from multiline to single-line and vice-versa making the code reviews harder when you want to keep a multiline for readability reason and it becomes a single-line, and in a future when a new parameter is added it becomes again multiline.

data class Foo(
    val a: A,
    val b: B,
)

That becomes:

data class Foo(val a: A, val b: B)

And if you add more params:

data class Foo(
    val a: A,
    val b: B,
    val c: C,
    val d: D,
    val e: E,
)

The format on constructor has changed too, personally, I prefer the old one, which follows the "square" pattern:

- public open class HubdleExtension
- @Inject
- constructor(
-     project: Project,
- ) : HubdleEnableableExtension(project) {

+ public open class HubdleExtension @Inject constructor(project: Project) :
+     HubdleEnableableExtension(project) {

[darioseidl]

Looks like this is intentional, going by the comments in the code

ktfmt/core/src/main/java/com/facebook/ktfmt/format/FormattingOptions.kt

Lines 50 to 52 in 5ff9fbb

	* Lists that cannot fit on one line will have trailing commas inserted. Lists that span
	* multiple lines will have them removed. Manually inserted trailing commas cannot be used as a
	* hint to force breaking lists to multiple lines.

(Btw. that comment seems to have a mistake. I guess it should say, "Lists that fit on one line will have them removed.")

ktfmt/core/src/main/java/com/facebook/ktfmt/format/TrailingCommas.kt

Lines 64 to 78 in cec9b50

	* Record elements which should have trailing commas inserted.
	*
	* This function determines which element type which may need trailing commas, as well as logic
	* for when they shold be inserted.
	*
	* Example:
	* ```
	* fun foo(
	* x: VeryLongName,
	* y: MoreThanLineLimit // Record this list
	* ) { }
	*
	* fun bar(x: ShortName, y: FitsOnLine) { } // Ignore this list
	* ```
	*/

Personally, I would also prefer if anything with two arguments would be formatted on multiple lines with trailing commas.

Edit: after using 0.52 a bit, I feel strongly that more than one argument should go on separate lines. The whole point of trailing commas is to make merge conflicts easier, isn't it? That should not depend on whether it fits on one line.

[LandonPatmore]

I rolled back to even 0.50 and am seeing this constructor snafu showing up.

We had this:

class RealSettingsWorkflow @Inject constructor(
  private val oAuthTokenService: OAuthTokenService,
  @UserId private val userId: String,
  @VersionName private val versionName: String,
) : StatefulWorkflow<Unit, SettingsState, RootOutput, BodyAndOverlaysScreen<Screen, Overlay>>() {

is now:

class RealSettingsWorkflow
@Inject
constructor(
  private val oAuthTokenService: OAuthTokenService,
  @UserId private val userId: String,
  @VersionName private val versionName: String,
) :
  StatefulWorkflow<Unit, SettingsState, RootOutput, BodyAndOverlaysScreen<Screen, Overlay>>() {

I am using spotless, but I don't think it is causing this since it delegates to ktfmt. As an ex-Square, I do agree that the Square method is much easier to read. This current format it both ugly and makes me have to move my eyes down to see important information.

[nreid260]

Trailing comma management was initially flag guarded behind --google-style. The point of the feature is to enforce consistency, by removing the ability for devs to hint at formatting preference. Any proposal to change the behaviour needs to maintain that property for --google-style.

The specific rules on when to remove/insert were designed-by-committee based on vibes. Keeping short lists to one line is broadly a choice to reduce vertical space (but we're absolutely not trying to minimize vertical space).

---

In our experience, lots of people hated the new behaviour the first few times it rearranged their existing code. Then about two weeks later they got used to it, and went on with writing code.

We also don't want to experience that pain more times than necessary, so there's a strong bias toward keeping the existing arbitrary rules, unless there's some, more objective, better approach.