Is NewClientWithEnvProxy necessary?

[WillAbides]

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:

1. 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"

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

[gmlewis]

So, unfortunately, I'm the one who proposed it here as a workaround.

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.

[WillAbides]

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

[gmlewis]

Yeah, me too. Maybe we can get some comments from the other contributors who are interested.