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

Improve auto type (List or Object) for CSV to JSON conversion #11

Closed
tuanchauict opened this issue Apr 29, 2021 · 3 comments · Fixed by #12
Closed

Improve auto type (List or Object) for CSV to JSON conversion #11

tuanchauict opened this issue Apr 29, 2021 · 3 comments · Fixed by #12

Comments

@tuanchauict
Copy link
Contributor

In RetrosheetInterceptor.isReturnTypeList(), I see we are using contains("java.util.List") to check whether the return type is in list type or not. This limits auto type to just List but not the other collection type like Set, Array.

Besides, suspend function does not return a list but a kotlin.coroutines.Continuation param is appended at the last of the parameter list. It seems to me that the code cannot check suspend function.

Using contains also dangerous because if a generic type (not collection) accepts List as the type param, contains will cause incorrect. For example:

fun foo(): Foo<Bar<List<String>>>

One more problem (although we don't expect to use retrosheet for a real product), obfuscation may rename List to anything..

I suggest a new check like this:

private fun isSuspendMethodWithListReturnValue(method: Method): Boolean {
    val lastParameter = method.genericParameterTypes.lastOrNull() ?: return false
    val typeName = lastParameter.typeName
    val continuationClassName =
        kotlin.coroutines.Continuation::class.qualifiedName ?: return false
    val isSuspendFun = typeName.startsWith("$continuationClassName<")
    if (!isSuspendFun) {
        return false
    }
    // TODO: We can shorten this with Regex
    val suspendFunReturnType = typeName
        .removePrefix("$continuationClassName<")
        .split("<")
        .first()
        .split(" ")
        .last()
    val clazz = Class.forName(suspendFunReturnType)
    return Collection::class.java.isAssignableFrom(clazz) || clazz.isArray
}
@tuanchauict tuanchauict changed the title Improve auto type (List or Object) for JSON Improve auto type (List or Object) for CSV to JSON conversion Apr 29, 2021
@tuanchauict
Copy link
Contributor Author

It also seems to me that Method::class.java.getDeclaredField("signature") works on JVM but not on Android.
I could run it for Unit test but cannot run on Android app.

@theapache64
Copy link
Owner

You're right. To fix this issue, I've recently created a new annotation called @ReadAsList. But I think this is much better and avoids the usage of extra annotation.

@theapache64
Copy link
Owner

Will be available from 2.0.0-alpha05.

@tuanchauict Thank you so much for this solution 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants