Omit public modifier on override function or constructor parameters #1550
Conversation
…ifiers contain override modifier
|
If this looks good, should I push a commit to format the TypeSpecTest.kt? It's not nicely formatted now. |
Because it explicitly added public modifier
| .build(), | ||
| ) | ||
| .build() | ||
| // We cannot omit the public modifier here. The bytecode is different. |
There was a problem hiding this comment.
Not sure I understand what "The bytecode is different" means in this context.
There was a problem hiding this comment.
Here is what it means. Generated by Kotlin Bytecode from IntelliJ IDEA
There was a problem hiding this comment.
Got it. If the intent is to explain why the "public" modifier can't be omitted, I don't think the reference to bytecode is very helpful. I'll provide suggestions on how to reword this, but please let me know if I misunderstood your intent.
We run Spotless on CI, so the build would've failed had this file been misformatted. |
Will clean it up in a separate PR
I have used Spotless for a long time too. The issue is ktlint somehow won't always cleanup/format imports. If we apply these changes, it won't break
Also, I think it's kind of ugly here, maybe we should also add
Maybe we can do this in a separate PR too. |
That's probably something we can/should report to ktlint to help get it fixed. In general, if IDE makes formatting changes that don't interfere with ktlint/spotless, it's fine to merge them. But I don't think it's very productive to apply this kind of formatting changes manually. That said, if you'd like to clean these up, we'll accept the PR.
Sure, but code styles are subjective. ktlint gives us good basic rules and will reject most glaring code style issues, and we don't care too much about things that ktlint doesn't catch. |
kotlinpoet/src/test/java/com/squareup/kotlinpoet/TypeSpecTest.kt
Outdated
Show resolved
Hide resolved
kotlinpoet/src/test/java/com/squareup/kotlinpoet/TypeSpecTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thank you, looks great!
Will merge this once you've signed our CLA, please see contributing.md.
|
Signed! |
Fix #1548