feat: Api response extensions#754
Conversation
…re data retrieving Add onXXX method to apply code when success or error happens with strong typing (not null) Add mapXXX method to transform response content
There was a problem hiding this comment.
Pull request overview
Adds Kotlin extension helpers to streamline handling of ApiResponse success/error cases (callbacks + mapping), and extends the core ApiResponse model with an isError() convenience method.
Changes:
- Introduces
ApiResponseextensions:onSuccess,onError,on,mapSuccess,mapError,mapwith Sentry reporting on invalid states. - Adds
ApiResponseException/ApiErrorExceptionused for reporting inconsistent API payloads. - Adds
isError()toApiResponsemodel.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| Network/src/main/kotlin/com/infomaniak/core/network/utils/apiResponse/ApiResponseExt.kt | New ApiResponse extension API for success/error callbacks and mapping with Sentry capture. |
| Network/src/main/kotlin/com/infomaniak/core/network/utils/apiResponse/ApiResponseException.kt | New exception for “success but data is null” cases. |
| Network/src/main/kotlin/com/infomaniak/core/network/utils/apiResponse/ApiErrorException.kt | New exception for “error but error payload is null” cases. |
| Network/Models/src/main/java/com/infomaniak/core/network/models/ApiResponse.kt | Adds isError() helper to support new extensions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Failed to generate code suggestions for PR |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨No code suggestions found for the PR. |
|
|
|
||
| import com.infomaniak.core.network.models.ApiResponse | ||
|
|
||
| class ApiErrorException(response: ApiResponse<*>) : Exception("Error is null but required for an error $response") |
There was a problem hiding this comment.
The name ApiErrorException doesn't really reflect what it's always about (according to the exception message). Same for ApiResponseException.
Nothing in the class name tells you that it's about missing data.
I have multiple suggestions:
- Merge these 2 exceptions into one, with the name of any of them, and error message as a constructor parameter
- Create a sealed class hierarchy with the name
ApiResponseExceptionfor the base class, and 2 nested subclasses named something likeMissingErrorFieldandMissingDataField.
| } | ||
| } | ||
|
|
||
| inline fun <T : Any> ApiResponse<T>.on( |
There was a problem hiding this comment.
Will we really need this function?
I feel like its usages would not look idiomatic, and a bit redundant with repetition of the word on:
someResponse.on(
onSuccess = { whatever() },
onFailure = { somethingElse() }I think onSuccess and onError are good enough for our use cases.



No description provided.