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

Convert ApiResponse into a seal class. #63

Merged
merged 9 commits into from
Nov 15, 2023

Conversation

lucaslabari
Copy link
Contributor

Goal

  • Convert ApiResponse into a seal class.
  • This way ApiClient will always return an ApiResponse of type Success, Failure or Error.

Testing

Updated unit tests.

@lucaslabari lucaslabari requested a review from a team as a code owner November 13, 2023 21:26
Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Merging #63 (5935e61) into master (2695940) will decrease coverage by 0.15%.
Report is 12 commits behind head on master.
The diff coverage is 43.75%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
- Coverage   76.06%   75.91%   -0.15%     
==========================================
  Files         309      311       +2     
  Lines       10046    10094      +48     
  Branches     1447     1461      +14     
==========================================
+ Hits         7641     7663      +22     
- Misses       1817     1841      +24     
- Partials      588      590       +2     
Files Coverage Δ
.../embrace/android/embracesdk/comms/api/ApiClient.kt 0.00% <ø> (ø)
...mbrace/android/embracesdk/comms/api/ApiResponse.kt 71.42% <71.42%> (-3.58%) ⬇️
...race/android/embracesdk/comms/api/ApiClientImpl.kt 72.72% <52.94%> (-6.44%) ⬇️
.../android/embracesdk/comms/api/EmbraceApiService.kt 69.47% <29.16%> (-12.58%) ⬇️

... and 20 files with indirect coverage changes

Copy link
Collaborator

@bidetofevil bidetofevil left a comment

Choose a reason for hiding this comment

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

A question overall: we do a lot of logging all over this class. Is it really necessary? Given how many API requests we make, and how standard they all are, we would get a lot of noise that isn't particularly useful

@lucaslabari
Copy link
Contributor Author

A question overall: we do a lot of logging all over this class. Is it really necessary? Given how many API requests we make, and how standard they all are, we would get a lot of noise that isn't particularly useful

There was already a lot of logs, I thnk we can review which of them makes sense and remove the others on separate PR.

Copy link
Collaborator

@bidetofevil bidetofevil left a comment

Choose a reason for hiding this comment

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

LGTM

@lucaslabari lucaslabari merged commit 4c5ff31 into master Nov 15, 2023
2 of 4 checks passed
@lucaslabari lucaslabari deleted the lucas/refactor_api_response branch November 15, 2023 17:40
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.

2 participants