ktlint icon indicating copy to clipboard operation
ktlint copied to clipboard

Consider a rule to normalize annotation style

Open JakeWharton opened this issue 2 years ago • 6 comments

Expected Rule behavior

Enforce multiple annotations use array-form, individual-form, but not both at the same time in the same location.

// GOOD:
@A @B @C
fun sup() {}
// GOOD:
@[A B C]
fun sup() {}
// BAD:
@A @[B C]
fun sup() {}

I suspect people will also want to enforce one style over the other globally. That is to say, if there are multiple annotations at a site they must either use the @[A B] form or @A @B form. I'm not sure enforcement of one style or the other by default makes sense, but that's a conversation we can have with the community.

The Android style guide says that array-form can only be used with a declared annotation receiver and provided they do not have arguments (https://developer.android.com/kotlin/style-guide#annotations). I'm not sure this very specific restriction is worth fully honoring, but it's important to note since it's somewhat an argument against a global default style but not fully.

Some interesting cases to handle:

// GOOD, separate receiver
@A @field:[B C]
val string: String
// GOOD, separate explicit receivers
@set:[A B] @get:[C D]
val string: String
// BAD, should be combined to a single array
@get:[A B] @get:C
val string: String

JakeWharton avatar Aug 09 '23 19:08 JakeWharton

This is an interesting proposal.

That is to say, if there are multiple annotations at a site they must either use the @[A B] form or @A @B form. I'm not sure enforcement of one style or the other by default makes sense, but that's a conversation we can have with the community.

I would definetly favor to implement a global code style. Consistency in code style is a key factor in making code in a project easy to read. It might be best to create a separate rule for grouping annotation into arrays versus ungrouping annotations from arrays. This rule indeed would need a setting with the preferred style. The style could be one of following:

  • ungroup (split arrays of annotations in individual annotations)
  • arrays (combine annotations for same receiver into an array with exception for annotations with parameters) Disabling the rule, allows you to use a mixed style.

I wouldn't know what the best default would be. I have the feeling that the array annotations are more heavily used in Android development. The default can be set per coding style.

paul-dingemans avatar Aug 12 '23 13:08 paul-dingemans

I would favor to have each array of annotation to be on a separate line. So

// BAD, array of annotation should go on separate line
@[A B] @get:[C D]
val string: String
// GOOD
@[A B]
@get:[C D]
val string: String

// BAD, array of annotation should go on separate line
@set:[A B] @get:[C D]
val string: String
// GOOD
@set:[A B]
@get:[C D]
val string: String

Is it acceptable, or even favoured, to reorder annotations? Given original code:

@B @A @field:[D C]
@F("foo")
@[E G]
val string: String

When reordering is not allowed, this would become:

@[B A]
@field:[D C]
@F("foo)
@[E G]
val string: String

If reordering is allowed, it could be written as:

@[A B E G]
@F("foo)
@field:[D C]
val string: String

where ordering is:

  • annotations without parameter and without receiver in alphabetical order combined in array
  • annotations with parameter(s), one per line
  • annotations with receivers but without parameters. Annotations for same receiver are combined in an array. Each annotation for that same receiver but having a parameter go on a separate line. The receivers are sorted alphabetically.

paul-dingemans avatar Oct 15 '23 18:10 paul-dingemans

I wouldn't know what the best default would be. I have the feeling that the array annotations are more heavily used in Android development.

If you want a single data point, I've never seen array annotations used, and I didn't even know it was a thing. I'm mostly work in Android, but do work on general JVM/KMP as well.

eygraber avatar Oct 15 '23 19:10 eygraber

We do use array notation for readability. Agree with Jake's original notation that we shouldn't mix the two on a single element, but imo both should be allowed.

I would argue against any kind of reordering.

shashachu avatar Oct 16 '23 18:10 shashachu

...we shouldn't mix the two on a single element, ...

I would argue against any kind of reordering.

This seems contractdictory to me. Given sample below:

@B @A @field:[D C]
@F("foo")
@E @G
val string: String

A, B, E and G are all annotations without receivers. Jake's propopal is that those should be grouped into a single array. That would mean that the order of the annotations will change.

we shouldn't mix the two on a single element, but imo both should be allowed.

This would mean that whenever an element is annotation with an annotation containing parameters, that all other annotations on that element should be ungrouped as well.

paul-dingemans avatar Oct 18 '23 08:10 paul-dingemans

Parking the issue for now, due to lack of input / consensus.

paul-dingemans avatar Nov 18 '23 13:11 paul-dingemans