kotlinpoet icon indicating copy to clipboard operation
kotlinpoet copied to clipboard

CodeBlock.endFlowControl shouldn't insert a new line by default

Open Egorand opened this issue 3 years ago • 7 comments

Probably a relict of JavaPoet where a control flow would almost certainly be followed by new line. With Kotlin, new line by default makes code like below look bad:

@Test fun controlFlowsAsExpressions() {
  val controlFlows = buildList { 
    repeat(2) { 
      add(
        CodeBlock.builder()
          .beginControlFlow("if (num %% 2 == 0)")
          .addStatement("println(%S)", "foo")
          .nextControlFlow("else")
          .addStatement("println(%S)", "bar")
          .endControlFlow()
          .build()
      )
    }
  }
  val listOfControlFlows = controlFlows
    .joinToCode(prefix = "listOf(⇥\n", separator = ",\n", suffix = ",⇤\n)")
  assertThat(listOfControlFlows.toString()).isEqualTo(
    """
    listOf(
      if (num % 2 == 0) {
        println("foo")
      } else {
        println("bar")
      }
      ,
      if (num % 2 == 0) {
        println("foo")
      } else {
        println("bar")
      }
      ,
    )
    """.trimIndent()
  )
}

Egorand avatar Jul 23 '22 19:07 Egorand

I think the counterpoint is when you're writing it as statements in a body so I'm not sure omitting is a great default either.

In Kotlin it actually feels like we should model more constructs like control flow as a type which you can either emit as a statement or part of an expression.

Another one that comes to mind in this space is that you cannot use beginControlFlow for a trailing lambda with a named argument because it handles emitting the curly.

JakeWharton avatar Jul 23 '22 22:07 JakeWharton

I think that addStatement should be responsible for emitting the new line, by default a control flow should be an expression. Maybe every CodeBlock should be considered an expression by default, and addStatement("%L", codeBlock) should be how it can be turned into a statement? It'd be interesting to explore introducing a type to represent control flows though.

Another one that comes to mind in this space is that you cannot use beginControlFlow for a trailing lambda with a named argument because it handles emitting the curly.

This actually is supported!

https://github.com/square/kotlinpoet/blob/a1163dafc72e5724c947c07a3bd006f6baba695d/kotlinpoet/src/test/java/com/squareup/kotlinpoet/CodeBlockTest.kt#L454-L468

Egorand avatar Jul 24 '22 01:07 Egorand

Tough to pull this off without breaking like 100% of users though. Might be time to rework the whole "code" part of the API.

JakeWharton avatar Jul 25 '22 18:07 JakeWharton

Another example where it's a problem that endControlFlow inserts a newline. If I want to generate this (Http4K) code:

public fun route(): ContractRoute {
    // Route
    return "/items" meta {
        summary = "Summary."
        description = "Description."
        returning(OK to "Success.")
    } bindContract GET to ::handler
}

This:

FunSpec.builder("route").apply {
    returns(ClassName("org.http4k.contract", "ContractRoute"))
    addComment("Route")
    beginControlFlow("return %S meta {", "/items")
    addStatement("summary = %S", "Summary.")
    addStatement("description = %S", "Description.")
    addStatement("returning(OK to %S)", "Success.")
    endControlFlow().addStatement("bindContract GET to ::handler")
}.build()

Actually generates this code with a newline before bindConstrant, which is invalid:

public fun route(): ContractRoute {
    // Route
    return "/items" meta {
        summary = "Summary."
        description = "Description."
        returning(OK to "Success.")
    }
    bindContract GET to ::handler
}

Virtlink avatar Jan 05 '23 12:01 Virtlink

That is not control flow. It's a multiline single statement.

JakeWharton avatar Jan 05 '23 13:01 JakeWharton