Do copies http.DefaultClient

[nostalgictesla]

By default builder.Do() uses http.DefaultClient, copied on every request. Lots of packages are very promiscuous in using that variable which could lead to very subtle bugs for users that are hard to figure out. My preferred expectation when using the package is that I will always get a "clean" client unless I specifically call builder.Client() to inject my own. I think it would be a good idea to remove the default call to global state and use a package private var defaultClient = http.Client{} instead?

[earthboundkid]

I like the current behavior. It’s standard. http.DefaultClient is the default client for Go applications. If you don’t want to use it, pass in your own. The whole point of having it is to be able to make a global change.

[nostalgictesla]

Thanks for the answer, I appreciate the design choice. I don't think use of those variables is considered standard anymore though. Most recent guidance I've seen advises against using these global state variables from the stdlib as they can be overwritten from anywhere and cause extremely hard to debug bugs. I happened to find this because we have a defensive lint config that checks specifically for usage of http.DefaultClient because in the past it has casued us serious problems when metrics and telemetry libraries have overwritten the http.DefaultClient for their own purposes and broken completely unrelated code relying on that variable. Similar bugs have happened with the log package globals.

I'll work around it if it is a done deal but it would definitely be a nice to have. I'm currently in the process of updating a few hundred kloc of Go code to use this package instead of net/http because it is so well designed that it eliminates the possibility of most of the HTTP request based bugs that we see in our code, but this is the one thing I've seen in the package during audit that would introduce some (albeit very small) cognitive overhead rather than remove it.

[earthboundkid]

You could write a standard config for your application that overwrites the client and whatever other settings you need and then have your linter require the use of myapp.RequestConfig.

[earthboundkid]

requests.New makes using a standard config nicer:

func myAppConfig(rb *requests.Builder) {
   cl := http.Client{ Timeout: 1 * time.Second }
   rb.
    BaseURL("https://whatever.example/v2")
    Client(&cl)
}

var foo FooRequest
err := requests.
  New(myAppConfig).
  Path("foo").
  ToJSON(&foo).
  Fetch(ctx)