-
Notifications
You must be signed in to change notification settings - Fork 60
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
v5: Sealed interfaces, Raw Retrofit Responses, Kotest Migration, Kotlin 1.6.0 #50
Conversation
Sealed interfaces allow for exhaustive when expressions evaluating only Success and Error.
* | ||
* when (response) { | ||
* is NetworkResponse.Success -> // Action Succeeded do something with body | ||
* If you expect your server response to not contain a body, set the success body type ([S]) to [Unit], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there should be a NetworkResponse.Complete
and there can be a discussion on if we should include
public val response: Response<*>
or not
we can also add a typealias of CompletableResponse
typealias CompletableResponse<E> = NetworkResponse<Unit, E>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've included the raw Response
as an escape hatch when you want more control over the response than what this library provides. I see no harm in adding it, but a loss of control for consumers in removing it. Unless there's a strong argument against not including it, it's better to leave it in there IMO.
Great idea on the CompletableResponse
, I'll add it as a nice utility.
Great!
On Wed, Dec 8, 2021 at 3:13 PM Leandro ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/main/kotlin/com/haroldadmin/cnradapter/NetworkResponse.kt
<#50 (comment)>
:
> */
- data class NetworkError(override val error: IOException) :
- NetworkResponse<Nothing, Nothing>(), Error
+ public sealed interface Error<S, E> : NetworkResponse<S, E> {
Should the errors be nested? and if they are nested can they be renamed to
not include the "Error" keyword? NetworkResponse.ServerError or
NetworkResponse.Error.Server instead of NetworkResponse.Error.ServerError`
Sorry, that was how I originally implemented it. I had not realised
@haroldadmin <https://github.com/haroldadmin> had *nested* the classes
already. I would agree with your last comment: we should neither nest nor
rename the classes in this release.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#50 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD563XCKXRTMAX5TIPKWF63UP7J7BANCNFSM5JMDNPRA>
.
--
Best regards,
*Gil Goldzweig Goldbaum*
T: +972 50-839-2942
E: ***@***.***
|
Appreciate the thorough review, @gilgoldzweig and @argenkiwi, the feedback is really useful. I'll update the PR with the requested changes. |
Other changes: - Improve documentation on the public API surface - Implement OkHttp timeout for faster test execution - Add tests for successful responses with errors during body deserialization - Add CompletableResponse type alias for NetworkResponse<Unit, E>
I've updated the pull request with your feedback. Would appreciate another round of review, @gilgoldzweig @argenkiwi. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fantastic, awesome job!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the good work @haroldadmin and apologies for the late reply. Looking forward to the next release. :D
This PR accomplishes some milestones aimed at the next major release of the library (#34)
Sealed Interfaces
The
NetworkResponse
class is now based on sealed interfaces. This allows for more concisewhen
expressions when you don't care about the specific type of the error:Raw Retrofit Responses
NetworkResponse.Success
andNetworkResponse.ServerError
now bundle the raw retrofitResponse<S>
object to allow for greater access to the response of the network request.We still supply
code
andheaders
properties as before to retain familiarity with the existing API design.Handling Empty Response Bodies
In the current version of the library you have to use the
Unit
response type if you expected your server to respond without a body. This is fine when the API never returns a body, but causes problems if it sometimes returns a body and sometimes doesn't (200 vs 204 status code).The bundled raw Retrofit responses provide a better way to handle this situation.
Tests Overhaul & Migration to Kotest
This PR gets refactors the library's test suite to get rid of redundant and obscure tests, and replaces them with a streamlined test suite focused on publicly exposed functionality.
We've also finally moved away from the deprecated
kotlintest
artifacts to the newkotest
libraries.Remove Deprecated Classes
We've removed deprecated ways to instantiate adapter factory. The existing classes had been marked as deprecated for a long period, and I hope everyone has moved away from them.
Kotlin 1.6.0
Updated the language level to 1.6.0
Sample App
Add a module showing sample usage of the library using the
kotlinx.serialization
library.