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

Add support for Kotlin's Result #3873

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

tomridder
Copy link

add the ResultCallAdapter to return Result to wrap the outcome (success/failure) of a retrofit call, instead of returning a value or throwing an exception. #3558

like

@get("me")
suspend fun getUser(): Result

all the testcase in Kotlin-test package and retrofit-adapter moudle's test case has been passed

@tomridder
Copy link
Author

@JakeWharton @codebutler hello ?anyone there

@@ -39,6 +37,8 @@ import java.io.IOException
import java.lang.reflect.ParameterizedType
import java.lang.reflect.Type
import kotlin.coroutines.CoroutineContext
import org.junit.Assert.*

Choose a reason for hiding this comment

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

Don't use star imports if this project wasn't already using them

@@ -23,6 +23,8 @@
import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;
import javax.annotation.Nullable;

import kotlin.Result;

Choose a reason for hiding this comment

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

extra space and new import not alphabetized

return try {
val response = delegate.execute()
val result = if (response.isSuccessful) {
Result.success(response.body()!!)

Choose a reason for hiding this comment

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

Don't use !! - there is always a better way to fail gracefully into the Result.failure case if this is null.

}
Response.success(result)
} catch (e: IOException) {
Response.success(Result.failure(e))

Choose a reason for hiding this comment

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

This does the same job as the more general Exception case, so it's not needed

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 this pull request may close these issues.

3 participants