You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I suspect that NewClientWithEnvProxy is redundant with NewClient(nil). The reason I think this is NewClient(nil) uses returns a Client with an empty http.Client. http.Client with no transport uses http.DefaultTransport, and http.DefaultTransport is configured to use ProxyFromEnvironment. So, NewClient(nil) returns a client that has the same proxy config as NewClientWithEnvProxy.
There are a couple of differences though:
NewClientWithEnvProxy's client uses a transport that is missing all the other config in http.DefaultTransport. While somebody might want this, I don't think it makes sense to get that out of a constructor named "NewClientWithEnvProxy"
Somebody could modify http.DefaultTransport so that it is unsuitable for use with go-github but then want to use an empty *http.Transport configured with ProxyFromEnvironment for go-github. This seems like an unlikely scenario, and in the case it comes up they can pass an http.Client to NewClient that is configured however they want.
This came up when working on #2897. If we agree that NewClientWithEnvProxy is unnecessary I can mark it deprecated with a note saying to use github.NewClient(nil) instead.
The text was updated successfully, but these errors were encountered:
If, after reading #2363 and #2686, you are still convinced that NewClientWithEnvProxy is unnecessary, I'm fine to deprecate it as you said with a clear explanation as to how to solve those two issues appropriately.
I think #2363 is only partially related. It is about configuring a proxy on the transport, but not from environment.
I'm confused by #2686. Presumably it solved an issue, but I'm not sure what. That's what makes me doubt my assertion that NewClientWithEnvProxy is redundant with NewClient(nil).
I suspect that
NewClientWithEnvProxy
is redundant withNewClient(nil)
. The reason I think this isNewClient(nil)
uses returns aClient
with an emptyhttp.Client
.http.Client
with no transport useshttp.DefaultTransport
, andhttp.DefaultTransport
is configured to use ProxyFromEnvironment. So,NewClient(nil)
returns a client that has the same proxy config asNewClientWithEnvProxy
.There are a couple of differences though:
NewClientWithEnvProxy
's client uses a transport that is missing all the other config inhttp.DefaultTransport
. While somebody might want this, I don't think it makes sense to get that out of a constructor named "NewClientWithEnvProxy"Somebody could modify
http.DefaultTransport
so that it is unsuitable for use with go-github but then want to use an empty*http.Transport
configured with ProxyFromEnvironment for go-github. This seems like an unlikely scenario, and in the case it comes up they can pass an http.Client to NewClient that is configured however they want.This came up when working on #2897. If we agree that NewClientWithEnvProxy is unnecessary I can mark it deprecated with a note saying to use
github.NewClient(nil)
instead.The text was updated successfully, but these errors were encountered: