ktfmt

ktfmt copied to clipboard

Original code:

  @Inject @Foo
  var bar: Bar

ktfmt removes line break:

  @Inject @Foo var bar: Bar

Android Kotlin style guide says annotations can only be on the same line as the declaration if there is only annotation (and without arguments): https://developer.android.com/kotlin/style-guide#annotations

This is also true for annotations with arguments:

  @Retention(SOURCE) @Target(FUNCTION, PROPERTY_SETTER, FIELD) annotation class Global

(See again the Android style guide, which suggests even a single annotation with an argument should be on a separate line.)

If this is the preferred ktfmt style, we might consider changing the Android style guide. But I think it's good to have a discussion about it at least.

cc @strulovich

If I recall correctly how it got to this, we had some issues with parameters. This looks very awkward when we break after annotations:

fun doIt(
  @Anno1 @Anno2 item1,
  @Anno3 @Anno4 item2) { ... }

Since it becomes:

fun doIt(
  @Anno1 @Anno2
  item1,
  @Anno3 @Anno4
  item2) { ... }

The second one looks very unnatural to me and I don't think people code this way. I'm open for updating the style, but then we need to split it according to the annotated element: field, variable, parameter, etc.

I think the example parameters in the previous comment are kind of a best-case scenario for the existing behavior because things line up neatly: every parameter has annotations of exactly the same length. When that's not the case, such as when annotating controller methods or payload classes with an annotation-based web framework, I think it's less obvious that breaking more aggressively would make things look unnatural.

For example, ktfmt does this:

fun doIt(
    @Annotation1 @Annotation2("some arguments") item1: Int,
    item2: String,
    @Annotation3 @Annotation4 item3: Double,
    @Annotation5
    @Annotation6("some arguments like say a bit of API documentation that cause line wrapping")
    item4: Boolean,
    @Annotation1 item5: Int
) { ... }

as opposed to the proposed behavior in this issue, which would left-align the arguments:

fun doIt(
    @Annotation1
    @Annotation2("some arguments")
    item1: Int,
    item2: String,
    @Annotation3
    @Annotation4
    item3: Double,
    @Annotation5
    @Annotation6("some arguments like say a bit of API documentation that cause line wrapping")
    item4: Boolean,
    @Annotation1
    item5: Int
) { ... }

There's a workaround which I've taken to using in cases where I find the existing formatting especially hard to read. You get line breaks if you put a // comment after the last annotation, like so:

fun doIt(
    @Annotation1
    @Annotation2("some arguments") //
    item1: Int,
    item2: String,
    @Annotation3
    @Annotation4 //
    item3: Double
) { ... }

It's ugly but in some cases less ugly than the default.

@strulovich FWIW, I prefer annotations broken out per-line for parameters as well (like @sgrimm). But I would not be opposed to leaving them inline if it'd mean getting annotations for functions consistently on the previous line.

In a recent comparison between ktfmt and kotlin-auto-formatter, this was the primary issue we found where ktfmt produced lower-quality code.

is there any progress?

There's no active work on this.

@rattrayalex, I'd appreciate a few real examples of where it produces uglier code as reference.

@strulovich

@strulovich I'm not developing in this area actively right now, but if you run ktfmt over this repo you should see a number of diffs related to annotations.

@strulovich I've come across the following:

@PlanningEntity
class LoadJob {
    @PlanningId lateinit var id: String

    @InverseRelationShadowVariable(sourceVariableName = "tour") var vehicle: Vehicle? = null

    @PreviousElementShadowVariable(sourceVariableName = "tour") var previousLoadJob: LoadJob? = null

    @NextElementShadowVariable(sourceVariableName = "tour") var nextLoadJob: LoadJob? = null
}

which definitely reads worse than

@PlanningEntity
class LoadJob {
    @PlanningId
    lateinit var id: String

    @InverseRelationShadowVariable(sourceVariableName = "tour")
    var vehicle: Vehicle? = null

    @PreviousElementShadowVariable(sourceVariableName = "tour")
    var previousLoadJob: LoadJob? = null

    @NextElementShadowVariable(sourceVariableName = "tour")
    var nextLoadJob: LoadJob? = null
}

In this case I'd strongly consider using the workaround with trailing comments provided by @sgrimm

Moved from ktlint to ktfmt and this was one of the major eyesores for me.
I feel like in-line for parameters is fine (readable) but in-line for top-level fields / methods should be 1-per line.

We started using ktfmt, and this is the only thing that I don't like about it. Whenever there are multiple annotations, or annotations with parameters, or multiple annotated properties or parameters, I prefer annotations on their own lines. Since that's so many cases, just having them always on their own lines would be fine too.

Here's another related issue regarding annotations with parameters: https://github.com/facebook/ktfmt/issues/316 and https://github.com/facebook/ktfmt/issues/409

Agreed on strongly preferring multiple annotations to be split out one per line. Simple, no-params, single annotations can be kept on the same line, but I'd personally also prefer them on a separate line if possible, for consistency and readability.