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
933 stars 78 forks source link

Google style with trailing comma does not use maxWidth correctly #472

Open Rawa opened 6 months ago

Rawa commented 6 months ago

Hello!

There seems to be an off-by-one error when using Google-style and trailing commas. I just tried out the Google Style with trailing commas and saw the following scenario:

    @Test
    fun testShowBillingErrorPaymentButton() =
        composeExtension.use {
            // Arrange
            setContentWithTheme {
                AccountScreen(
                    state =
                        AccountUiState.default()
                            .copy(billingPaymentState = PaymentState.Error.Billing),
                    uiSideEffect = MutableSharedFlow<AccountViewModel.UiSideEffect>().asSharedFlow(),
                )
            }

            // Assert
            onNodeWithText("Add 30 days time").assertExists()
        }

The line: uiSideEffect = MutableSharedFlow<AccountViewModel.UiSideEffect>().asSharedFlow(), ends up being 101 characters even though I set maxWidth to 100.

If i change the named parameter to uiSideEffects, we see the following behavior, where it correctly wraps the line:

    @Test
    fun testShowBillingErrorPaymentButton() =
        composeExtension.use {
            // Arrange
            setContentWithTheme {
                AccountScreen(
                    state =
                        AccountUiState.default()
                            .copy(billingPaymentState = PaymentState.Error.Billing),
                    uiSideEffects =
                        MutableSharedFlow<AccountViewModel.UiSideEffect>().asSharedFlow(),
                )
            }

            // Assert
            onNodeWithText("Add 30 days time").assertExists()
        }

My configuration is as follows:

    configure<com.ncorti.ktfmt.gradle.KtfmtExtension> {
        googleStyle()
        blockIndent.set(4)
        continuationIndent.set(4)
        maxWidth.set(100)
        removeUnusedImports.set(true)
    }
nreid260 commented 5 months ago

The 1.0 issue mentioned merging trailing-comma management into all styles. If we do that, I think this issue is unblocked.

We could switch the default behaviour of trailing-commas to do "nothing", rather than today where they trigger a line-break. Once that's done, management becomes:

Since the insertion happens before pretty-print, all the elements are already at their maximum length. We might get some lines that are unnecessarily wrapped after removal, but that's much less noticeable than exceeding max-width.

nreid260 commented 3 months ago

As of v0.52 trailing comma management appears to be on for all styles. Is there any. The proposal above can be implemented. @hick209

jdemeulenaere commented 2 months ago

This issue also happens with --kotlinglang-style now that this style enables trailing comma management. Any chance this can be fixed soon? It makes ktlint unhappy and complain about line width in AOSP :-)

nreid260 commented 2 months ago

I implemented the idea in https://github.com/facebook/ktfmt/issues/472#issuecomment-2166182024 and its not good.

Imagine a case like foo.bar(a, b).baz(a, b).fiz(a, b). There are 3 param lists, so 3 commas are preemptively inserted, which might push the length past the max line length. That would be sort of mediocre on its own, but worse, pretty printing breaks as the .s not the ,s, so all the inserted commas end up removed. Then it's surprising why this code, which should fit on one line, is forced to break.


I'd like to revive https://github.com/facebook/ktfmt/pull/473. It's got less surprising output and was also easier to implement.

greyhairredbear commented 1 month ago

I'd like to revive https://github.com/facebook/ktfmt/pull/473. It's got less surprising output and was also easier to implement.

@hick209 Does Nick's comment above change your perspective on #472?

(I'm also facing this issue after upgrading to 0.52)

nreid260 commented 1 week ago

Bump @hick209