-
Notifications
You must be signed in to change notification settings - Fork 769
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
Extend PolymorphicJsonAdapterFactory to allow custom handling of unknown labels #1094
Extend PolymorphicJsonAdapterFactory to allow custom handling of unknown labels #1094
Conversation
7242c01
to
c81b2ab
Compare
Ping @JakeWharton, @ZacSweers as last developers taking care of merging PRs. As mentioned in the issue I'm working on Yelp/swagger-gradle-codegen#103 and having this merged would simplify a lot the process as well as it would solve #784 by providing flexibility to developers of managing the "edge-case" of unknown labels as they like. I'd like to have some feedbacks around this PR or at least an ETA 😄 . |
c81b2ab
to
a184e1a
Compare
…faultValue, defaultValueSet)
a184e1a
to
9fad705
Compare
Sorry for the delay. In the future - tagging individual users based on perceived activity isn't the best approach and would appreciate if you don't do that. I like the idea behind this approach, but have a few thoughts:
|
@ZacSweers , sorry for the direct ping. I apologize for that.
Ideally yes, as already visible on the PR the method does return a
I understand your point, but consider the following case in Yelp/swagger-gradle-codegen#103 class Animal(open var type: String)
data class Dog(override var type: String, var name: String): Animal(type=type)
data class Cat(override var type: String, var size: Int): Animal(type=type) If we would allow "custom" handling on unrecognized labels we might have The above example is mostly meant to provide a realistic use-case where we don't only need read but also writes. Said so I'm open to ideas to implement it differently, but I thought that giving the freedom to inject a Before I do update the PR by annotating |
Are there any updates on this? |
@macisamuele I like the idea, lets go forward with this. |
Small follow-up: #1182 |
Nice PR by the way. I apologize for being slow-to-respond. The last several months have been... hectic. |
This PR includes an implementation proposal for #784.
The main idea is to allow users of the
JsonAdapterFactory
to provide aJsonAdapter
that will deal with thereader
in case of an unknown/unmapped label.Doing this has the advantage to give freedom to developers to re-use already existing components and tools to build their logic.