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 Do methods #38

Closed
wants to merge 4 commits into from

Conversation

vthiery
Copy link
Contributor

@vthiery vthiery commented Jun 11, 2018

[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
Copy link
Contributor

rShetty commented Jun 22, 2018

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

@vthiery
Copy link
Contributor Author

vthiery commented Jun 27, 2018

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

@rShetty
Copy link
Contributor

rShetty commented Jul 16, 2018

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

@vthiery
Copy link
Contributor Author

vthiery commented Jul 19, 2018

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

@rShetty
Copy link
Contributor

rShetty commented Mar 25, 2019

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

@vthiery
Copy link
Contributor Author

vthiery commented Mar 25, 2019

@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.

@vthiery vthiery mentioned this pull request Mar 25, 2019
@rShetty rShetty closed this Jan 27, 2020
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.

None yet

2 participants