-
Notifications
You must be signed in to change notification settings - Fork 37
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
Allow Code-geneated Moshi Adapters #82
Allow Code-geneated Moshi Adapters #82
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally good 👍 Still we have a couple of things to address:
-
You need to remove
.add(KotlinJsonAdapterFactory())
from theGeneratedCodeConverters
as @filipemp did here: https://github.com/Yelp/swagger-gradle-codegen/pull/68/files -
You need to provide Proguard rules to make sure that type adapters are not removed from the generated artifact.
samples/kotlin-android-with-moshi-kotlin-codegen/build.gradle.kts
Outdated
Show resolved
Hide resolved
Ideally if you could rebase your branch in top of Filipe's would be great
so we have his contribution in the graph as well.
…On Wed, Jan 22, 2020, 13:30 Samuele Maci ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In plugin/src/main/java/com/yelp/codegen/KotlinGenerator.kt
<#82 (comment)>
:
> + // Import JsonClass annotation to enable Adapters generations via Kotlin Annotation Processor
+ codegenModel.imports.add("com.squareup.moshi.JsonClass")
I totally missed the other PRs that were opened ... sorry :(
I'll check if we can move the import into the if statement, I'll let you
know soon about the progress.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#82?email_source=notifications&email_token=AAW44ZMM554CU3RMPSL4KYTQ7A375A5CNFSM4KKCMRNKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCST7IBA#discussion_r369532244>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAW44ZL57ZHMV5MADHZWBITQ7A375ANCNFSM4KKCMRNA>
.
|
Sure thing, as mentioned I did not see (I should have looked more carefully) PRs already trying to address the same issue. |
1a80090
to
6c453a8
Compare
Co-authored-by: Filipe Pereira <fpereira@yelp.com>
Co-authored-by: Filipe Pereira <fpereira@yelp.com>
Co-authored-by: Filipe Pereira <fpereira@yelp.com>
…ed in the repo Co-authored-by: Filipe Pereira <fpereira@yelp.com>
6c453a8
to
10dd7df
Compare
@cortinico, @filipemp: I did re-push the changes as I made the pre-commit hooks configuration less greedy ;) |
Can we have the re-formatting + pre-commit change on a separate change? This makes checking out the PR Diffs for third party easier. |
@cortinico just to be sure, before publishing the changes, are you asking me to remove commits Definitely it will simplify reviewing this, after all the commits are applying automated fixes |
Co-authored-by: Filipe Pereira <fpereira@yelp.com>
…nto kapt generated code) Co-authored-by: Filipe Pereira <fpereira@yelp.com>
Yes correct. Let's keep the "infra" work outside of this PR. |
10dd7df
to
aa38f00
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm but @cortinico has way more context on the global scale than I do so I'd wait for his review
The goal of this PR is pretty straight forward it plugs-in
moshi-kotlin-codegen
into the generated code.The main difference in the code generated by swagger-gradle-codegen is the presence of
@JsonClass(generateAdapter = true)
within the code.This annotation allows KotlinJsonAdapterFactory to identify the JsonAdapter generated via kotlin annotation processor (kapt) if the kapt directive (look into
samples/junit-tests/build.gradle
changes) is enabled.This approach leads to a no-op change for users of the current tool but empowers them to get better runtime performances by enabling kapt.
Disclaimer: I've not verified the performance improvements but considering that this is an opt-in approach I'm not concerned about it
Fixes #68
Fixes #62
Fixes #71