Use kotlin.jvm annotations if jvmInterop #3502
Conversation
|
Okay looking at that failure, sorry. I have |
wire-kotlin-generator/src/main/java/com/squareup/wire/kotlin/KotlinGenerator.kt
Outdated
Show resolved
Hide resolved
cbrunsdon
force-pushed
the
kotlin_jvm_annotations_on_jvm_interop
branch
from
35f6be9 to
b10aaa5
Compare
*Most* places were guarded in an:
```
if (javaInterOp) {
propertyBuilder.jvmField()
}
```
But the adapter and a few choice places were not.
Doing this with the boolean adds a bit of readability to these calls, as
it wasn't very clear if it was expected or accidental with the changes
to javaInterop checking before.
*Theoretically* we could remove these annotations in these places, but
they're required for things like the GSON generator that references the
ClassName#Adapater, so not now.
Again, not entirely sure the context of why this is always a jvm field, but we're going to explicitly leave it
I don't quite know the context of the android creator, but it was always added previously so leaving it so.
These can be moved pretty cleanly one-to-one with what was there before.
Everything moved over now, removing the old one.
I think the theoretical reason for needing the `internal.Jvm*` annotations is someone could choose to generate files outside of a common*/jvm*/main src directory, which wouldn't have the kotlin.jvm annotations defined. However, if javaInterop is requested, use the kotlin annotations instead. This works better in Some Places(tm) where other things care about those annotations that some nerds might care about. Like maybe some lunatic who depends on an included gradle project mixing both kotlin multiplatform and not...
Like @JvmField before, *if* we're building with JavaInterop we don't need the internal as we can be Quite Sure we're building into an environment kotlin will provide these annotations to us.
Doing these at the end for clarity, though the previous versions are still all valid against the previous commits.
cbrunsdon
force-pushed
the
kotlin_jvm_annotations_on_jvm_interop
branch
2 times, most recently
from
6c95693 to
3cf40aa
Compare
|
Okay, I updated the inline annotations and pushed the "which package" decision up, due to The one thing I'd draw attention to due to unfamiliarity is I had to remove I think that sample is pretty contrived and old enough its probably not something someone would need to care about. |
wire-kotlin-generator/src/main/java/com/squareup/wire/kotlin/KotlinSchemaHandler.kt
Outdated
Show resolved
Hide resolved
|
|
||
| typeToKotlinName.putAll(BUILT_IN_TYPES) | ||
|
|
||
| val jvmAnnotationPackage = if (javaInterop) "kotlin.jvm" else "com.squareup.wire.internal" |
There was a problem hiding this comment.
Can we move it inside KotlinGenerator so as to not change KotlinGenerator's API ?
Just below its constructor,
private val jvmAnnotationPackage = if (javaInterop) "kotlin.jvm" else "com.squareup.wire.internal"and that's good enough
There was a problem hiding this comment.
I had it there originally but the javaInterOp (not javaInterop) is overridden when the constructor is called
return KotlinGenerator(
schema = schema,
profile = profile,
typeToKotlinName = typeToKotlinName,
memberToKotlinName = memberToKotlinName,
emitAndroid = emitAndroid,
javaInterOp = javaInterop || buildersOnly,I can move it back but I'll have to push the logic of that one deeper. I'm fine to make the change but its just differently ugly, but the private constructor on KotlinGenerator makes changing the API of it reasonably safe and easy to move around at will.
There was a problem hiding this comment.
Okay, I pushed it back inside and delayed the javaInterop || buildersOnly, can pull that commit off easily enough if you want it swapped back.
Just something that is nice to know if you're reading this.
buildersOnly will set javaInterOp = true, which *could* be set by someone that building in an enviroment that included `kotlin.jvm`. We'll still use the internal jvm* flags, though they can't get any use out of the @jvm* functionality.
Explicitely separating from the last commit, the constructor is private so this should be fine, with the `synthetic` there.
Feedback request on the PR.
cbrunsdon
force-pushed
the
kotlin_jvm_annotations_on_jvm_interop
branch
from
3cf40aa to
9543d54
Compare
Doing this before the constructor was called meant we couldn't know in the Generator if interop was explicitely requested or came to us through buildersOnly
cbrunsdon
force-pushed
the
kotlin_jvm_annotations_on_jvm_interop
branch
from
9543d54 to
fa75505
Compare
2 participants
There is a bit of background around the
internal.Jvm*annotations here for background:#2504
They have two (minor?) issues:
@Jvm*are treated special, and certain setups of module dependencies and project layouts can cause IDE auto-completion, for instance, to not interpret theinternal.Jvmcorrectly, redlining your perfectly-reasonable and runnable codeinternalbut they're public aliases which isn't amazing to ship to any projects including wire-runtime and other careless programmers (totally not me though...) might accidentally auto-complete those in instead of their expected@Jvmannotations.I don't actually quite follow the how/when/why the
internal.Jvm*was required. It might be a historical issue that generating into /main wouldn't have those provided? That no longer seems to be the case as settingjvmAnnotationPackagein this commit to kotlin.jvm seems to run the suites just fine?This change sets the annotations to reference the
kotlin.jvmannotations if jvmInterop is enabled, which we should expect to be provided by anyone generating while explicitly asking for jvm compatibility.Either way, though this adds complexity to the generation process, I personally believe its worth it as a step to phase these out.