Description
As I was working on #2895 I realized that go-github has four different funcs for creating a new github.Client depending on what options you want to set.
- NewClient sets the
http.Client - NewClientWithEnvProxy builds an
http.Clientconfigured withhttp.ProxyFromEnvironment - NewTokenClient builds an
http.Clientconfigured to use bearer token auth. - NewEnterpriseClient is like unto NewClient but lets you set urls that point to your GHE server.
These are all fine on their own, but you can't combine them. If you want to configure a client for your enterprise server that uses token authentication, you need to choose whether to use NewTokenClient then configure BaseURL and UploadURL or configure an http.Client for token auth and pass it to NewEnterpriseClient.
Proposal
Create a new method on *Client: WithOptions(opts ...ClientOption) (*Client, error) along with options that cover the existing constructors' functionality.
The options would be:
WithAuthToken(token string)WithEnvProxy()WithEnterpriseURLs(baseURL, uploadURL string)1
Creating a new enterprise client with an auth token would look something like:
client, err := github.NewClient(nil).WithOptions(
github.WithEnterpriseURLs(myURL, myURL),
github.WithAuthToken(myToken),
)Alternatives Considered
Update NewClient to accept options
This would involve changing NewClient's signature to:
func NewClient(httpClient *http.Client, opts ...ClientOption) *ClientThis would be a backward-compatible signature change because the only new argument is variadic. This would also probably be the best option if we could limit it to just this change. However, not all options are guaranteed to be valid, so we would need to either add an error to the signature or panic when an invalid option is passed. Neither of those are good compromises, so I moved on from this.
Add a "New" func
Client is the primary export of the github package, so a constructor named "New" would be a good fit semantically. If "New" doesn't work for a name, then "NewClientWithOptions" could work too.
Being a new function would also allow us to avoid the required http.Client argument and move that to an option called WithHTTPClient.
The Signature would look like: New(opts ...ClientOption) (*Client, error)
Creating a new enterprise client with an auth token would look something like:
client, err := github.New(
github.WithEnterpriseURLs(myURL, myURL),
github.WithAuthToken(myToken),
)This seems like a compelling option to me. I prefer the actual proposal over this because this issue started with an observation that we have too many constructors. Adding a new constructor in response reminded me of xkcd 927.
Separate With* methods for each option
Instead of one WithOptions method, we could add WithEnterpriseURLs, WithTokenAuth, etc.
I think a single WithOptions method lends itself to better usage. This might be a better option if none of the methods returned errors so proper method chaining could be used, but that isn't the situation here.
Footnotes
-
I chose this over separate WithBaseURL and WithUploadURL options because this makes it more clear that they will be modified the same way NewEnterpriseClient currently modifies urls. ↩