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

added a new format specifier (%U) for literals that should never wrap #1860

Closed

Conversation

seanreid-toast
Copy link

@seanreid-toast seanreid-toast commented Mar 7, 2024

Using KotlinPoet, we found that the existing format specifiers weren't quite appropriate for our use case.

In some cases, we have to generate classes that have large byte arrays. Because of the way those are compiled later, they can't be simple bytes arrays (they get encoded as instructions rather than ending up in the constant pool), and we often hit the large method limit. Instead, we encode them as strings. We take the responsibility of escaping characters, and we don't want any line wrapping to make decoding easier.

We tried to use %S, but the automatic escaping and using multiline strings made guaranteeing the correctness of later decoding difficult.

We tried to use %L, but the automatic line wrapping caused compilation failures and also introduces decoding risk for us.

We didn't want to change behavior of existing specifiers, so adding a new one for a literal without wrapping seemed like the right course. Locally testing, it appears to do exactly what we need and cause no regressions in other functionality. The new specifier is %U for "unbroken" literals.

  • docs/changelog.md has been updated if applicable.
  • CLA signed.

@Egorand
Copy link
Collaborator

Egorand commented Mar 9, 2024

You can use the non-wrapping space character, ·, if you need a literal that doesn't wrap:

@Test fun nonWrappingLiterals() {
  val wrapMe = FunSpec.builder("wrapMe")
    .returns(STRING)
    .addStatement(
      "%L",
      "very long string very long string very long string very long string very long string very long string very long string very long string very long string very long string very long string very long string"
        .replace(' ', '·'),
    )
    .build()
  assertThat(toString(wrapMe)).isEqualTo(
    """
      |package com.squareup.tacos
      |
      |import kotlin.String
      |
      |public fun wrapMe(): String {
      |  very long string very long string very long string very long string very long string very long string very long string very long string very long string very long string very long string very long string
      |}
      |
    """.trimMargin(),
  )
}

Introducing a specialized format specifier feels redundant to me.

@seanreid-toast
Copy link
Author

Introducing a specialized format specifier feels redundant to me.

Thanks for the feedback @Egorand, I definitely understand the point about this change being potentially redundant.

If you don't want to merge this PR, that's okay with me. However, I do think there are a few reasons why something like this, despite its redundancy, might be valuable to the project and should be considered.

  1. Discovery of the line wrapping middle-dot concept is difficult. It's documented in docs/functions.md and not mentioned anywhere in docs/l-for-literals.md, where it might be more discoverable. I don't think I'm alone in missing it given the number of issues reported about line wrapping behavior (non-exhaustive links).

  2. The concept of · does generally seem backwards to me in the case of %L. %L is for literals, but to prevent kotlinpoet from modifying the string that supposed to be literal, a developer must themselves modify the string so that kotlinpoet can reverse that modification it to emit the string literally. I think either reversing the behavior in a future major version (as proposed in #1314) or using a solution similar to one in this PR would be appropriate to better as it would eliminate the transient modify-to-unmodify workflow.

All that being said, if this PR isn't a direction you'd like to go, I'd understand. If you're open to me taking a pass at the docs to try to make discovery of · simpler instead, I'd be willing to propose something.

@Egorand
Copy link
Collaborator

Egorand commented Mar 14, 2024

I think your use case is rather unusual, where you essentially have a string, but you can't use it with %S, so you're resorting to %L, and need to perform extra processing to tell the library that what you're passing in is actually a string - so I definitely understand that this feels backwards. From the 5 issues you linked, only #1753 is somewhat similar, but the use-case there is different; other issues are either bugs that were resolved, or failure to use %S for string literals. I do think that #1314 is the long term solution, though it likely requires a major version bump due to its impact.

In the general spirit of making common things easy and making less common things possible, the library does provide you with tools to work around the limitation you have, but I don't think it warrants dedicated API to support an uncommon use case. Regarding extra documentation, we would surely welcome PRs. I must mention though that the main use case for · is to control wrapping in constructs that KotlinPoet does not model as strictly, such as function bodies; in literals, %S prevents auto-wrapping, and %L expects input that either can't be wrapped or is safe to wrap.

If you'd like an extra set of eyes on the issues you're having with %S, I'm more than happy to take a look - please feel free to open a discussion and share relevant code snippets.

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

Successfully merging this pull request may close these issues.

2 participants