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.

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

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

Open
Rawa opened this issue Jun 4, 2024 · 5 comments · Fixed by meslubi2021/ktfmt#13
Open

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

Rawa opened this issue Jun 4, 2024 · 5 comments · Fixed by meslubi2021/ktfmt#13

Comments

@Rawa
Copy link

Rawa commented Jun 4, 2024

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)
    }
@Rawa Rawa changed the title Google style with trailing comma does not maxWidth correctly Google style with trailing comma does not use maxWidth correctly Jun 4, 2024
nreid260 added a commit to nreid260/ktfmt that referenced this issue Jun 4, 2024
@nreid260
Copy link
Contributor

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:

  • Insert trailing-commas everywhere
  • Pretty print
  • Remove unnecessary trailing-commas

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

nreid260 commented Aug 15, 2024

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

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

I implemented the idea in #472 (comment) 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 #473. It's got less surprising output and was also easier to implement.

@greyhairredbear
Copy link
Contributor

I'd like to revive #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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants