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

Annotate with @JsonClass classes with no parameters/variables #88

Conversation

macisamuele
Copy link
Collaborator

This is needed because if we would fully remove moshi-kotlin dependency (as planned by version 2 release) we would not be able to properly have the Json to Object conversion and vice-versa.

The most important change introduced by this PR is visible into samples/junit-tests/.../models/EmptyModel.kt.

The reason for forcing the presence of @JsonClass(generateAdapter = true) even if there are no attributes is related to the fact that if we would remove the usage of KotlinJsonAdapterFactory then we would not be able to covert the EmptyModel instance from/to JSON.

In order to validate it I've run this script.
The script basically ensures that we have no moshi-kotlin/kotlin-reflect dependencies on samples/junit-tests and runs the tests

The results (raw results in here), as expected, shows that:

  • PR branch leads to green tests
  • current master fails to run emptyEndpointTest_withEmptyBody test.
    The test failure contains the following exception (the raw exception is available here)
java.lang.IllegalArgumentException: Unable to create converter for class com.yelp.codegen.generatecodesamples.models.EmptyModel
...
Caused by: java.lang.IllegalArgumentException: Cannot serialize Kotlin type com.yelp.codegen.generatecodesamples.models.EmptyModel. Reflective serialization of Kotlin classes without using kotlin-reflect has undefined and unexpected behavior. Please use KotlinJsonAdapter from the moshi-kotlin artifact or use code gen from the moshi-kotlin-codegen artifact.

@macisamuele macisamuele force-pushed the maci-support-moshi-codegen-empty-class-edge-case branch from 24eae7b to 96e290a Compare February 3, 2020 16:02
@macisamuele macisamuele added the enhancement New feature or request label Feb 3, 2020
Copy link
Collaborator

@cortinico cortinico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but please update the tests. Also having #61 fixed would be great to catch this scenario in the future.

@cortinico cortinico added this to the 1.4.0 milestone Feb 3, 2020
@cortinico
Copy link
Collaborator

LGTM!

@macisamuele macisamuele merged commit 9de9741 into Yelp:master Feb 4, 2020
@macisamuele macisamuele deleted the maci-support-moshi-codegen-empty-class-edge-case branch February 4, 2020 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants