omit more permissive modifiers on restricted visibility types #1881
Conversation
j-roskopf
force-pushed
the
jr/omit_visibility
branch
from
b1cb281 to
70f3355
Compare
|
I would like to see the tests cover more cases. Basically four classes with each of the four visibilities, each having four functions or properties with the four visibilities. I'm sure many of the cases are covered implicitly in other tests, but having one that actually covers all the cases specific to this functionality would be nice. |
|
@JakeWharton Updated tests to add better coverage for the matrix you mentioned. I added tests to cover 5 classes (public, private, protected, internal, unspecified) with properties + functions over the same 5 |
j-roskopf
force-pushed
the
jr/omit_visibility
branch
from
0d23625 to
41c1be9
Compare
|
|
||
| protected fun protectedStringMethod(): String = "taco" | ||
|
|
||
| public fun publicStringMethod(): String = "taco" |
There was a problem hiding this comment.
A bunch of these tests seem incorrect with the goal. In my original bug there's a private class showing that the public modifier isn't needed.
There was a problem hiding this comment.
I wanted to verify that if any access modifier was specified, it was respected and added to the property / function even if the contain class was private / internal / protected / unspecified / public. So I added a matrix of those 5 each containing a function across the same 5, with the main change in behavior being that where the class is private / internal / protected, unspecifiedStringMethod has no access modifier specified in the source code because no modifier was specified through kotlin poet.
please let me know if i'm way off base!
kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt
Outdated
Show resolved
Hide resolved
kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt
Outdated
Show resolved
Hide resolved
j-roskopf
force-pushed
the
jr/omit_visibility
branch
from
eac3b24 to
617f5a8
Compare
|
Bump. I'd like to see this merged ASAP, please. And there are merge conflicts to resolve first. |
|
@JakeWharton please let me know if you thought my original implementation was off base from what you were thinking. happy to address the conflicts and bring the PR to a mergable state |
closes #1301
docs/changelog.mdhas been updated if applicable.