search

cli / go-gh

A Go module for interacting with gh and the GitHub API from the command line.
https://pkg.go.dev/github.com/cli/go-gh/v2
MIT License
322 stars 45 forks source link

Consider exposing a raw http.Client #13

Closed chelnak closed 2 years ago

chelnak commented 2 years ago

Hey - this is a great project and helped me get up and running quickly!

I was looking at integrating https://github.com/google/go-github in to my project. It allows you to pass a pre-configured http client when creating a new GitHubClient instance. The abstraction (RESTClient) doesn't implement the interface so cannot be used as a parameter.

Would you consider exposing a configured raw http.Client instance or the config package to allow us to build our own?

I'd be happy to help out with the implementation if it's something of use.

samcoe commented 2 years ago

@chelnak This seems like a good idea and we would be open to adding something like this. When designing go-gh we did not think of this use case so I expect that a couple things will need to change to get this to behave as expected by end users.

First, would be that specifying the Host in ClientOptions would no longer work as expected, as in your use case go-github would actually be responsible for directing the request to the correct host.

Second, is the header setting code we use right now will override any headers that are already set on the request so if go-github or another library wanted to set a header our code would overwrite it.

These would not be challenging to address but from a quick glance at https://github.com/cli/go-gh/pull/14 it appears neither is. Additionally, my view of the implementation would be a top level function rather than adding to the RESTClient interface. Something like:

func HTTPClient(opts *api.ClientOptions) (http.Client, error)

Let me know if taking on this work interests you.

chelnak commented 2 years ago

Hey! Thanks for the response. Yeah I'd love to give it a shot.

I'll process your comment a bit more and may come back with some clarification questions if that's alright?

chelnak commented 2 years ago

Hey @samcoe - i've just opened a new PR that will hopefully start to address your comments - tests still to be pushed.

Regarding your first one about specifying Host in ClientOptions. While it wont be used in other libraries the property is used by go-gh to fetch the auth token from config if it is stored under a different name. Maybe it's worth leaving it in?

What are your thoughts?