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

Improve wrapping logic of parameter lists #274

Open
Egorand opened this issue Nov 7, 2017 · 9 comments
Open

Improve wrapping logic of parameter lists #274

Egorand opened this issue Nov 7, 2017 · 9 comments
Assignees
Milestone

Comments

@Egorand
Copy link
Collaborator

Egorand commented Nov 7, 2017

Currently we wrap based on the number of parameters, not on the length:

val file = FileSpec.builder(tacosPackage, "Taco")
    .addFunction(FunSpec.builder("test")
        .addParameter("a", Int::class)
        .addParameter("b", Int::class)
        .addParameter("c", Int::class)
        .addStatement("return a + b + c")
        .build())
    .build()

prints this:

fun test(
    a: Int,
    b: Int,
    c: Int
) = a + b + c

when it could be this:

fun test(a: Int, b: Int, c: Int) = a + b + c
@swankjesse
Copy link
Collaborator

When we wrap any parameters we should wrap all parameters. I've got an idea for a fix: when we emit the %W in a parameter list we could include a group ID to identify related %W breaks. That way the line wrapper could decide whether to wrap all or none.

@Egorand
Copy link
Collaborator Author

Egorand commented Nov 7, 2017

We'll need a specialized line wrapper that can backtrack through the buffer and wrap all %W with that group ID.

@swankjesse
Copy link
Collaborator

Exactly. And I think we can just require groups to be contiguous.

@Egorand
Copy link
Collaborator Author

Egorand commented Nov 9, 2017

A problem with attaching a group ID to %W tokens is that there's no indication of how many tokens there are in the group and which one is the last one. To overcome this, I suggest introducing a new pair of placeholders, %( and %) that will contain any number of %W placeholders. The line wrapper will buffer everything inside the brackets, and when it hits the closing %), it will process the buffer, replacing placeholders with proper chars, and flush it. If any of %Ws has wrapped, %( and %) will transform into '\n', otherwise they'll turn into ''.

@Egorand
Copy link
Collaborator Author

Egorand commented Nov 29, 2018

#532 should help with this

@JakeWharton
Copy link
Collaborator

JakeWharton commented May 14, 2022

Thanks, I hate it!

        public fun ComoseGenerationTestRow(scoped: @Composable RowScope.() -> Unit, unscoped: @Composable
            () -> Unit): Unit {

I might be angry enough to rage fix this (it broke a test case).

@JakeWharton
Copy link
Collaborator

Here's my idea: a reverse BufferedSource.peek(). You buffer() a CodeWriter and write whatever you want to it but it goes to a StringBuilder not the real Appendable. Then, you inspect properties on it (like what line you're on). If you want, you can flush() back to the original, or you can simply discard and start over.

This way we'll call buffer(), write params, check if wrapped, if so re-do with one per line, if not flush().

@Egorand
Copy link
Collaborator Author

Egorand commented May 16, 2022

Yeah, I recall trying a similar approach back in 2018, buffering the output first to understand whether it will wrap or not, and using that knowledge to output to the real Appendable with or without wrapping. Please rage fix!

@JakeWharton
Copy link
Collaborator

The wrapping policy applies unconditionally to lambdas which is not great.

  public fun mixed(mixed: (
    i: Int,
    Long,
    s: String?,
  ) -> Unit)

These should probably not be subject to the policy and wrap normally.

@psymoon psymoon self-assigned this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants