Proposal: client.WithOptions

[WillAbides]

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.Client configured with http.ProxyFromEnvironment
• NewTokenClient builds an http.Client configured 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)¹

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) *Client

This 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

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

[gmlewis]

I like it! Sounds good to me.
Thank you, @WillAbides !

[gmlewis]

Hey, one quick question - what do you think about this?

client, err := github.NewClient(nil).
	WithEnterpriseURLs(myURL, myURL).
	WithAuthToken(myToken)

[gmlewis]

Oh, I see, that is your last option... hmm... pipelines come to mind:
https://pkg.go.dev/github.com/bitfield/script
but not sure we want the added dependency.

[WillAbides]

Oh, I see, that is your last option... hmm... pipelines come to mind: https://pkg.go.dev/github.com/bitfield/script but not sure we want the added dependency.

That's exactly it. I'm not familiar with that package, but it looks interesting. From what I can tell, the pipeline would store errors until they are checked for later. I think that would be kind of complex and lend itself to the common issue we see with bufio.Scanner where people forget to check scanner.Err() when they are done iterating. I know I have made that mistake a few times.

[gmlewis]

Agreed. I think you can proceed with your top-level proposal unless we hear objections from other users.