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

proposal: net/http: provide RoundTripperFunc adapter for RoundTripper #21509

Closed
frojasg opened this issue Aug 17, 2017 · 6 comments

Comments

Projects
None yet
5 participants
@frojasg
Copy link
Contributor

commented Aug 17, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go1.8.1

What operating system and processor architecture are you using (go env)?

GOOS="darwin"

What did you do?

N/A

What did you expect to see?

Implement an adapter function for http.RoundTripper would be helpful.
I see there's already one in the code for a test.
Could we add this to the http package?

What did you see instead?

@odeke-em odeke-em changed the title Provide RoundTripperFunc adapter for http.RoundTripper proposal: net/http: provide RoundTripperFunc adapter for RoundTripper Aug 18, 2017

@gopherbot gopherbot added this to the Proposal milestone Aug 18, 2017

@gopherbot gopherbot added the Proposal label Aug 18, 2017

@odeke-em

This comment has been minimized.

Copy link
Member

commented Aug 18, 2017

@frojasg thanks for the proposal, I personally don't have much of an opinion on how to judge criteria for adding this func in

type RoundTripperFunc func(*http.Request) (*http.Response, error)
func (fn RoundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) {
return fn(req)
}

but I'll ping @tombergan @ianlancetaylor @broady @rsc

@frojasg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2017

I would love to make this contribution if you think is worth adding this func.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2017

To add to the package, you'd need to motivate that this is so common that it would help real code. Do you have any stats on how often this comes up? My guess would have been that many RoundTrippers are parameterized by state and so they couldn't use this. (Also I don't think there are too many RoundTrippers at all, in contrast to http.Handlers.)

Introducing HandlerFunc was certainly a good thing, but each time you do introduce a FooFunc then you split the world into things using Foo and things using FooFunc. This is why we don't have io.ReaderFunc and so on.

@frojasg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2017

Do you have tools to know how often this comes up? When is this uses case often enough to add it to the package?

Looking at Github, I can tell that 5% of the people using http.Client is using http.RoundTripper. Furthermore, there are 351 cases where people are already using the RoundTripperFunc Adaptor.

Do you think this initial numbers could motivate doing further investigation? If so, I would love to get some guidance where to go next. In the other hand, If you still think the cost is too high. We could close this ticket.

@tombergan

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2017

My guess would have been that many RoundTrippers are parameterized by state and so they couldn't use this.

I would have the same guess. I looked at the first page of @frojasg's 351 cases. There are three categories of results:

  1. A utility package that defines RoundTripperFunc but doesn't actually use it anywhere. One example, and I cannot find an actual use of that package, although I probably fail at github search. This category of results is not useful.

  2. A test that defines a stub RoundTripper. One example.

  3. Code that defines a wrapper RoundTripper. One example. In each case like this one, the RoundTripperFunc does use state, but the state happens to come from the parent function context.

I'd be interested in knowing how often the third case happens in real code. Perhaps you could compute stats from rsc's go corpus? I did a quick search of Google's internal codebase and found that about 0.3% of custom RoundTrippers are a RoundTripperFunc. All other custom RoundTrippers are structs.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2017

The evidence seems clear that this is not worthwhile.

@rsc rsc closed this Aug 28, 2017

@golang golang locked and limited conversation to collaborators Aug 28, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.