Added [NetworkResponse.Error] an interface to generalize errors

[gilgoldzweig]

There are cases where I don't care what was the type of the error, I just want to know that there was an error without specifying the type each time

[haroldadmin]

Thank you for this PR.

I've gone through the code and I like how it simplifies generic error handling. I'm not sure if I'm a fan of adding even more components to the NetworkResponse class.

The crux of these changes to is to enable access to a error property when NetworkResponse is not Success. To enable this we are forcefully adding an error property to NetworkResponse.ServerError even though there's no exception there. I understand that it's necessary, but it pollutes the class definition.

I think extension properties might be the way to go here, as they allow extension of the class without modifying its definition:

when (val response = api.foo()) {
  is NetworkResponse.Success -> { /* ... */ }
  else -> {
    val error = response.error // Extension property on `NetworkResponse` class
    /* ..handle error */
  }
}

val NetworkResponse.error: Throwable?
  get() = when (this) {
    is NetworkResponse.Success -> null
    is NetworkResponse.ServerError -> Exception(this.body?.toString())
    is NetworkResponse.NetworkError -> error
    is NetworkResponse.UnknownError -> error
  }

What do you think this design?

[gilgoldzweig]

I see your point but I don't fully agree with it, let me explain why.

First about the ServerError doesn't hold an exception.
I think it should, even if we build it from the parameters we receive because it is an error after all and I may want to use it.

A possible use case is an event logger, I want to know and report that there was an error and be able to see it in my tracking because an error means the user wasn't able to complete the action as intended therefore I want to know why.

Regarding the extension property.
I didn't think about it and it's a cool method to solve the issue, but it introduce new issues.

The property can be accessed regardless of the result, so even if the action was successful, where there isn't an error value.

To solve that you have to make it nullable
Which means I lose the compile time safety and I have to run two checks.

"Was the action not successful and is the error not null", now I know it can be run by calling
error?.let {}

But one of the great things about your library is the removal of the checks by leveraging the kotlin compiler.

[haroldadmin]

I'm still in split minds about this change. I'll take some time to think about it, and get back to you by this weekend.

[gilgoldzweig]

That's fine, I think all changes be beneficial, now it's just a design question and that's up to you, it's your creation. I can vouch for my suggestion because I currently use it, but again it's totally up to you. I would anyway be happy to be a contributor

[gilgoldzweig]

Hi, So I wanted to see an implementation with the new kotlin 1.5 sealed interface feature and It looks really really good and I'll explain why.

I used different names to not cause confusion but that's just for testing.

Given a new class

sealed interface Response<out T, out U> {

    sealed interface Error<out U> : Response<Nothing, U> {
        val error: Throwable
    }

    data class Success<T : Any>(
        val body: T,
        val headers: Headers? = null,
        val code: Int
    ) : Response<T, Nothing>

    data class ServerError<U : Any>(
        val body: U?,
        val code: Int,
        val headers: Headers? = null
    ) : Error<U> {
        override val error = IOException("Network server error: $code \n$body")
    }

    data class NetworkError(override val error: IOException) : Error<Nothing>

    data class UnknownError(
        override val error: Throwable,
        val code: Int? = null,
        val headers: Headers? = null,
    ) : Error<Nothing>
}

The following cases compile and when returns a value:

fun example(response: Response<Int, BaseResponse>) {

    val arbitraryResult: Int = when (response) {
        is Response.Success -> 1
        is Response.Error -> 2
    }

    val arbitraryResult1: Int = when (response) {
        is Response.Success -> 1
        is Response.Error -> {
            when (response) {
                is Response.NetworkError -> 2
                is Response.ServerError -> 3
                is Response.UnknownError -> 4
            }
        }
    }

    val arbitraryResult2: Int = when (response) {
        is Response.Success -> 1
        is Response.NetworkError -> 2
        is Response.ServerError -> 3
        is Response.UnknownError -> 4
    }
}

But the following cases won't compile because the when doesn't return a value because it is missing the UnknownError

val arbitraryResult2: Int = when (response) {
    is Response.Success -> 1
    is Response.NetworkError -> 2
    is Response.ServerError -> 3
}

In addition, because Response.Error is a sealed interface No one outside can implement it.
This means this won't compile

class NewError(override val error: IOException) : Response.Error<Int>

Here is a small change that makes the Response.Error#error nullable so ServerError can be nullable

sealed interface Response<out T, out U> {

    sealed interface Error<out U> : Response<Nothing, U> {
        val error: Throwable?
    }

    data class Success<T : Any>(
        val body: T,
        val headers: Headers? = null,
        val code: Int
    ) : Response<T, Nothing>

    data class ServerError<U : Any>(
        val body: U?,
        val code: Int,
        val headers: Headers? = null
    ) : Error<U> {
        override val error = body?.let { Exception("$it") }
    }

    data class NetworkError(override val error: IOException) : Error<Nothing>

    data class UnknownError(
        override val error: Throwable,
        val code: Int? = null,
        val headers: Headers? = null,
    ) : Error<Nothing>
}

And when you use the error of the general Response.Error it will be nullable same goes for Response.ServerError, but for Response.NetworkError and Response.UnknownError the value will not be nullable
and of course Response.Success doesn't have any access to the error

So you can use as much or as little information as needed

I'm not saying you should change it to this but I just wanted to share another approach and I would love to hear your thoughts

[haroldadmin]

Huh, the sealed interfaces approach does look nice. We might be able to switch to it in a v5 release, since it's a big breaking change.

[gilgoldzweig]

Awesome, do you want me to open a v5 branch with this change and some cleanup of the deprecated classes?

[haroldadmin]

I don't know if Github allows non-maintainers to push code to a public repository. You can create the branch in a fork and I'll be happy to collaborate there!

[haroldadmin]

I've had more time to think about the changes in this PR now, and they make a lot of sense. I've been thinking of alternate ways to get the same result, but none of them are necessarily better than this approach.

Thank you for this contribution. We can discuss the sealed interfaces implementation in another issue.

[gilgoldzweig]

Nice, Good to know. I'm still thinking of new things I want to add as part of a full new version

[haroldadmin]

Excited to see them! I have a few more changes I want to do before releasing a new version, such as upgrading Kotlin and Coroutines versions. Expect a new release next weekend, but until then you can try a snapshot release from Jitpack.

[alonsd]

I've had more time to think about the changes in this PR now, and they make a lot of sense. I've been thinking of alternate ways to get the same result, but none of them are necessarily better than this approach.

Thank you for this contribution. We can discuss the sealed interfaces implementation in another issue.

I saw you merged the commit, but I can't seem to find the relevant changes when using the Master branch. Am I missing something?

[haroldadmin]

Are you sure you're inspecting the correct branch? I can see the changes in master here

[alonsd]

Are you sure you're inspecting the correct branch? I can see the changes in master here

In my build.gradle(:app) I have the following line -

implementation "com.github.haroldadmin:NetworkResponseAdapter:4.1.0"

But I can't use NetworkResponse.Error class

[haroldadmin]

There's no stable release with these changes yet. If you need to use these changes please use this pre-release version:

implementation 'com.github.haroldadmin:NetworkResponseAdapter:d9912f3dfe'

These changes will be released in v4.2.0 soon, along with a few dependency updates. Once that release is out, you can switch to the stable version again.

[alonsd]

There's no stable release with these changes yet. If you need to use these changes please use this pre-release version:

implementation 'com.github.haroldadmin:NetworkResponseAdapter:d9912f3dfe'

These changes will be released in v4.2.0 soon, along with a few dependency updates. Once that release is out, you can switch to the stable version again.

Great! Thanks. Any time estimation for v4.2.0?

[haroldadmin]

Soon ✌️

[haroldadmin]

v4.2.1 is out now with this feature. Thanks @gilgoldzweig!