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

Consider a rule to normalize annotation style #2172

Open
JakeWharton opened this issue Aug 9, 2023 · 6 comments
Open

Consider a rule to normalize annotation style #2172

JakeWharton opened this issue Aug 9, 2023 · 6 comments
Labels
annotation-grouping rule Parked Issue is not likely to be solved or has no clear resolution yet

Comments

@JakeWharton
Copy link
Contributor

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
@paul-dingemans
Copy link
Collaborator

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
Copy link
Collaborator

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.

@eygraber
Copy link
Contributor

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.

@shashachu
Copy link
Contributor

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.

@paul-dingemans
Copy link
Collaborator

...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
Copy link
Collaborator

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

@paul-dingemans paul-dingemans added Parked Issue is not likely to be solved or has no clear resolution yet and removed up-for-grabs labels Nov 18, 2023
@paul-dingemans paul-dingemans removed this from the 1.1 milestone Nov 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
annotation-grouping rule Parked Issue is not likely to be solved or has no clear resolution yet
Projects
None yet
Development

No branches or pull requests

4 participants