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

Improve error handling of coroutines API #373

Merged
merged 18 commits into from Jul 7, 2018

Conversation

@lucasvalenteds
Copy link
Collaborator

commented Jul 3, 2018

This pull request is indented to address the issue #365. This makes the coroutines functions to behave like the standard API regarding how the client should handle errors and exceptions.

To make sure that they're consistent, the awaitObject function was deprecated in favor of awaitSafelyObjectResult (#369) because it seems not to be possible to handle serialization/deserialization exceptions.

The test suite was also refactored to add missing test cases. Although there are none tests to verify the error handling of serialization/deserialization of awaitResponse and awaitString because I don't know how to make them fail.

Standard API Equivalent API using Coroutines Error type Handling method
response awaitResponse FuelError fold
responseString awaitString FuelError fold
responseObject awaitObject FuelError fold
--- awaitSafelyObjectResult FuelError fold
--- awaitResponseResult Exception try-catch
--- awaitStringResult Exception try-catch
--- awaitObjectResult Exception try-catch

Please, let me know if you have any comments.

lucasvalenteds added 14 commits Jul 3, 2018
* For standard API errors are passed wrapped in FuelError instance
* For methods ending with Result the errors are thrown as Exception
* Add missing test case for awaitResponse method
* Rename test cases to try to clarify it's behavior
* Change their order
* Sort them using the pattern: success test and failure test
* Rename them to describe what they are verifying
* Make it throw any exceptions instead of return it
* Clean up success test case
* Add missing exception handling test case
* Move it up
* Catches unexpected exceptions
Wrap the HTTP calls in an try-catch that fails if any Exception is
thrown.
* Add try/catch block to success case
* Rename methods to following the naming pattern
* Provide more descriptive failing messages
@codecov

This comment has been minimized.

Copy link

commented Jul 4, 2018

Codecov Report

Merging #373 into master will decrease coverage by 0.02%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #373      +/-   ##
============================================
- Coverage     77.14%   77.12%   -0.03%     
  Complexity      193      193              
============================================
  Files            33       33              
  Lines           919      918       -1     
  Branches        154      154              
============================================
- Hits            709      708       -1     
  Misses          129      129              
  Partials         81       81
Impacted Files Coverage Δ Complexity Δ
...com/github/kittinunf/fuel/coroutines/Coroutines.kt 65.21% <66.66%> (-1.45%) 0 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eae4b12...11b40e3. Read the comment docs.

@kittinunf

This comment has been minimized.

Copy link
Owner

commented Jul 5, 2018

This looks really awesome! Thanks for your contribution!! I think your change makes sense. Regarding to awaitResponse and awaitString, I think they are might be just okay to not have test.

If you don't mind,can you please help show simple usage in (super long) Readme.md so that people can see how it's supposed to be used 🙏

Again, thanks you are awesome!!

@lucasvalenteds

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 5, 2018

Thank you, @kittinunf. I appreciate the feedback :)

Also, the README file was updated with a section for coroutines module. The code has the runBlocking so people can just copy and paste and see it working. Let me know if it's good enough. 👍

@markGilchrist

This comment has been minimized.

Copy link
Collaborator

commented Jul 5, 2018

@lucasvalenteds I have raised a pr for the branch I was working on

It looks like you have covered some of the ideas discussed in the channel in your pr, if you see anything in my pr you want to use please feel free it not I will junk it #374

* Fixes typo on awaitObject deprecation message
* Fixes code style of deprecation annotation
@kittinunf

This comment has been minimized.

Copy link
Owner

commented Jul 7, 2018

@lucasvalenteds Yes, it looks awesome. 🍻

@markGilchrist do you think if we merge this one, your PR in #374 will still be valid? I feel kinda leaning towards the idea of not providing too many public interfaces for clients to use.

@markGilchrist

This comment has been minimized.

Copy link
Collaborator

commented Jul 7, 2018

@kittinunf I think that most of the changes in my pr are covered in the one

If you merge this one in we will be able to see if mine adds any value (I think that it may not) in which case the changes in my can be discarded

If this suits you and @lucasvalenteds?

@kittinunf

This comment has been minimized.

Copy link
Owner

commented Jul 7, 2018

@markGilchrist I see. Let's merge this in then.

@kittinunf

This comment has been minimized.

Copy link
Owner

commented Jul 7, 2018

@lucasvalenteds are you happy with the current status? if you do not yell out, I will assume that you are. 😸

@lucasvalenteds

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 7, 2018

@kittinunf This PR seems done to me. The other one sent by @markGilchrist I'll take a look in a moment.

@kittinunf kittinunf merged commit e7095ee into kittinunf:master Jul 7, 2018
1 of 3 checks passed
1 of 3 checks passed
codecov/patch 66.66% of diff hit (target 77.14%)
Details
codecov/project 77.12% (-0.03%) compared to eae4b12
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.