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

(WIP Prototype) Switch to public kotlinx-metadata API #570

Closed
wants to merge 8 commits into from

Conversation

@ZacSweers
Copy link
Collaborator

commented Jun 4, 2018

This swaps out kotlin-metadata for the new kotlinx-metadata API

I think this is actually done, but I'm unable to get the build working in the kotlin/tests project due to MetadataExtensions services not being visible for some reason. They work in the codegen/src/test tests though.

The high level architecture:

  • Switch to new Data class representations of specific types, replacing the raw protos they were reading before.
  • The metadata.kt class has been switched to provide APIs for resolving TypeNames from the library's KmVisitor APIs. This is a significant departure from before since it's a visitor pattern, but ended up working nicely once I wrote TypeNameKmTypeVisitor.
  • Shade the kotlinx-metadata library since it's not stable yet, and also avoid dependence on the kotlinx bintray repo

The other changes were pretty lightweight after migrating metadata.kt. Overall pretty happy with how this turned out, and definitely cleaner than reading the raw protos from before.

@@ -176,6 +186,11 @@
<pattern>com.squareup.kotlinpoet</pattern>
<shadedPattern>com.squareup.moshi.kotlinpoet</shadedPattern>
</relocation>
<!-- Repackage Kotlinx-metadata until its API is stable. -->
<relocation>
<pattern>kotlinx.metadata.jvm</pattern>

This comment has been minimized.

Copy link
@udalov

udalov Jun 11, 2018

I suppose you need to relocate kotlinx.metadata here, not kotlinx.metadata.jvm. I think this may fix the MetadataExtensions issue.

This comment has been minimized.

Copy link
@ZacSweers

ZacSweers Jun 11, 2018

Author Collaborator

Unfortunately this happens even if I disable shading

@ZacSweers

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 15, 2018

So an update - this will either need to wait for 0.0.3 of the library and possibly Kotlin 1.2.60, as the library's compiled against APIs that aren't available yet in kotlin 1.2.50

@ZacSweers

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 1, 2018

I have this moooostly working now in https://github.com/hzsweers/moshi/tree/z/kotlinxmetadata1260

I get strange issues with the runtime reflective lookup of adapters not working though. Wondering if kotlin 1.2.6x regressed/changes generated sources linking in kapt

@ZacSweers

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 1, 2018

Confirmed kapt sources location moved. Nearly there!

@ZacSweers

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 2, 2018

Continued in #642

@ZacSweers ZacSweers closed this Sep 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.