Improve Do methods - Githubissues

gojek / heimdall

An enhanced HTTP client for Go

http://gojek.tech

Apache License 2.0

2.63k stars 214 forks source link

Improve Do methods #38

Closed vthiery closed 4 years ago

vthiery commented 6 years ago

[This PR is more a proposal than anything else]

Hi!

This PR aims at simplifying the Do methods for both httpClient and hystrixHTTPClient. It also introduce multi-errors for the hystrixHTTPClient.

The reasons why this PR is a proposal more than anything are:

it changes the way errors are returned by the the hystrixHTTPClient,
it may slightly break retro-compatibility in case the client would want to use a non-nil response together with a non-nil error. As explained in detail here this might be the case in Go 1.

Let me know what you think 😉

rShetty commented 6 years ago

@vthiery Thanks for this PR. #33 will be merged into master. This PR might have to change

vthiery commented 6 years ago

@rShetty sure! Let me know when #33 gets merged and I'll adapt this PR

rShetty commented 6 years ago

@vthiery This will change after the new major changes to the master, this #33 , Please fix merge conflicts accordingly

vthiery commented 6 years ago

@rShetty I updated the PR. The CI pipeline fails but I believe this is due to a very short timeout used in tests.

rShetty commented 5 years ago

@vthiery Do you want to give a try to fix the conflicts ?

vthiery commented 5 years ago

@rShetty I'm not sure I can marry the valkyrie.MultiError with the recent changes on the hystrix client. Also, for the sake of clarity, I'd prefer to split this PR into two parts:

idiomatic, cosmetic changes
introduce valkyrie.MultiError in hystrix client

I can quickly take care of the first point but I may not have the time to work on the second point right now.