✨ Add support for custom Gson instances #645
Conversation
Add API support for passing custom Gson instances for serializing request entities or deserializing response body
|
Hey! Thanks a ton for this! Please allow me to take a look at the changes ✨ 🍻 . |
| * @param handler [ResponseResultHandler<T>] the handler that is called upon success | ||
| * @return [CancellableRequest] request that can be cancelled | ||
| */ | ||
| inline fun <reified T : Any> Request.responseObject(gson: Gson, noinline handler: ResponseResultHandler<T>) = |
There was a problem hiding this comment.
While we don't have anything particularly to against this, this obviously also does the job. But what do you think about what we have done in the fuel-moshi extension?
Here is the link to the master version
In short, in Moshi extension, we just expose the Moshi.Builder() directly. What do you think about it (exposing the GsonBuilder)? I think it might be just easier and consistent to do the same thing?
There was a problem hiding this comment.
Isn't Moshi.Builder() mutable? I'm afraid users can end messing things up, ex: you may want to decode same type from different APIs with different formats, I usually prefer to not expose unsafe APIs, but completely support you about consistency
Of course both options may be provided - shared mutable builder and explicit argument.
Also users may fallback to using gsonDeserializer(Gson) directly, in this case that function is not needed.
This is how my temporary solution currently looks:
inline fun <reified T : Any> Gson.deserializer(): ResponseDeserializable<T> =
object : ResponseDeserializable<T> {
override fun deserialize(reader: Reader): T? = fromJson<T>(reader, object : TypeToken<T>() {}.type)
}
Fuel
.get("path/to/my/service")
.timeoutRead(timeoutMillis)
.responseObject(gson.deserializer<List<MyResponse>>()) { result ->
Either.run {
result.fold(success = { cb(right(it)) }, failure = { cb(left(it)) })
}
}There was a problem hiding this comment.
That's true, I think you are right. While we are exposing the mutable builder, it provides the ease of use (with the power of users can mess things up of course). But what you said is correct about exposing the safe API is usually a better choice.
I think it depends on how much compromisation we would love to make. I think maybe exposing 2 APIs options also makes sense and giving flexibility.
As you are obviously one of the users! 😄, WDYT? In whatever way, we come to the conclusion, I will open a PR for moshi to standardize this.
There was a problem hiding this comment.
I vote for having explicit arguments for responseObject() functions and this will be consistent with FuelJackson implementation.
I would strongly discourage using shared builder, but ... :) - to keep it consistent - or need to add it to fuel-gson also, or deprecate it from other implementations - which is a breaking change: so this is more a decision for project owner, my view on the project is very limited.
We can keep it as is for now and it may be added later, when somebody request it: not giving it now - is ok ... but giving it now and getting it back later - will be a breaking change
There was a problem hiding this comment.
I think it's good to:
- deprecate shared builder (don't remove, yet), add default builder options to both (or all integrations)
- add function call time builder argument that defaults to a new builder using the default options.
In that case we can communicate to set the default options once and once only -- and we create a new builder each time --
it still has the issue that someone might mutate defaultOptions as the go, but that would be on them.
There was a problem hiding this comment.
Let's do that then? @lostintime would you mind doing us a favor of providing both options? It could be in separated PR though. We will be taking care of some other extensions.
There was a problem hiding this comment.
Sure, (was a bit busy these days :)
So, just to confirm - do you want me to add a val defaultGson = GsonBuilder() instance and mark it as deprecated from the beginning? or just add default value to gsonDeserializer(gson = Gson()) function, and use it everywhere needed?
There was a problem hiding this comment.
just add default value to gsonDeserializer(gson = Gson()) function, and use it everywhere needed?
Ah I meant this!
There was a problem hiding this comment.
I've moved Gson() instance to gsonDeserializer agrument.
I've also changed gsonDeserializerOf to:
inline fun <reified T : Any> gsonDeserializerOf(clazz: Class<T>) = gsonDeserializer<T>()It seemed redundant to me and there was one ore Gson instance used, but I'm not sure this is right, or maybe add one more gson argument here also, with default value?
There was a problem hiding this comment.
Yes, that sounds great with a default value. But I think I am happy to see this getting in. 🎉
fuel-gson/src/test/kotlin/com/github/kittinunf/fuel/FuelGsonTest.kt
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #645 +/- ##
===========================================
- Coverage 71.26% 71.07% -0.2%
Complexity 297 297
===========================================
Files 57 57
Lines 1486 1490 +4
Branches 193 193
===========================================
Hits 1059 1059
- Misses 335 339 +4
Partials 92 92
Continue to review full report at Codecov.
|
|
Let's merge this. |
kittinunf
changed the title
Add API support for passing custom Gson instances for serializing
request entities or deserializing response body
Description
Current
fuel-gsonimplementation have Gson instances hardcoded, I've added few overrides to existing functions to be able to pass a custom Gson instance.For some of new functions - default values may be used (vs override), but I'm not sure which one is safer for binary compatibility, hopefully you may give me a hint here, ex:
vs
Type of change
Check all that apply
Checklist: