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

Update ModifierOrderRule to include annotations as part of its checks #183

Merged

Conversation

JelloRanger
Copy link
Contributor

Annotations should precede all modifiers according to: https://kotlinlang.org/docs/reference/coding-conventions.html#modifiers

(It might make sense in the future to forbid having multiple annotations on the same line as the declaration per the link above. Not sure if that should be a new rule or not though; at the very least this should be an improvement over the existing check)

Had quite a bit of difficulty implementing the auto-format since annotation entries are a different type than all the other modifiers. I'm open to feedback on how to clean it up. :)

@shyiko
Copy link
Collaborator

shyiko commented Apr 8, 2018

Thank you for the PR, Jacob.
Any chance you could try preserving newline characters between annotations?
So that

@A
@B(v = [
    "foo", 
    "bar", 
    "baz"
])
@C
suspend public fun f()

wouldn't get squashed into

@A @B(v = [
    "foo", 
    "bar", 
    "baz"
]) @C public suspend fun f()

.

- The assumption that whitespace always separates modifiers isn't true
  since annotations can be on separate lines.
- Added test case to catch previous error
@JelloRanger
Copy link
Contributor Author

JelloRanger commented Apr 9, 2018

Good catch! I ended up playing around with it a bit and found a cleaner solution for the auto format that should fix this.

@shyiko
Copy link
Collaborator

shyiko commented Apr 10, 2018

Great!
One last thing before we merge this in.
Could you please update the PR to output

A.kt:3:1: Incorrect modifier order (should be "@annotation... public suspend")
A.kt:9:1: Needless blank line(s)

instead of

A.kt:3:1: Incorrect modifier order (should be "@A @B(
    "_"
) @C public suspend")
A.kt:9:1: Needless blank line(s)

given something like

//

@A
@B(
    "_"
)
@C suspend public fun f() {}


//

(otherwise this can break tools that depend on "one error per line")

- This is the guarantee that lengthy annotations or annotations with new
  lines don't cause an individual error to span multiple lines
@JelloRanger
Copy link
Contributor Author

Makes sense, done!

@shyiko
Copy link
Collaborator

shyiko commented Apr 11, 2018

Thank you, Jacob! 👍

@shyiko shyiko merged commit 4e58236 into pinterest:master Apr 11, 2018
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.

None yet

2 participants