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

Revert "change api http.client to interface" #1411

Merged
merged 1 commit into from
Dec 23, 2023
Merged

Conversation

bwplotka
Copy link
Member

@bwplotka bwplotka commented Dec 22, 2023

Hi,

Thanks @ArthurSens and @tips for doing the work! However, I think the #1387 change might not be necessary. I explained in #1387 (review) --let me know if this makes sense. I think we could update the docs to make it more clear.

TL;DR One can achieve the use case mentioned by @tips in many ways currently

api.Confg{Config: retryablehttp.StandardClient(}}

or

api.Confg{Config: &http.Client{Transport: <Your method with (*Request) (*Response, error) semantics>}}

Let's avoid additional interfaces which looks 99.999% like standard library ones and avoid allowing multiple ways of doing the same thing.

@bwplotka bwplotka requested a review from ArthurSens December 22, 2023 11:21
@bwplotka bwplotka mentioned this pull request Dec 22, 2023
@ArthurSens ArthurSens merged commit 96f1aec into main Dec 23, 2023
9 checks passed
@ArthurSens ArthurSens deleted the revert-1387-main branch December 23, 2023 13:41
ArthurSens pushed a commit that referenced this pull request Dec 23, 2023
Revert "change api http.client to interface"

Signed-off-by: Arthur Silva Sens <[email protected]>
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