Refactor `WithEnterpriseURLs` helper

[stevehipwell]

The current implementation of the WithEnterpriseURLs helper doesn't function as expected and doesn't appear to support GitHub Enterprise Cloud. For example testing the path of a url.URL can't assume that there will be slashes at either end. It also looks like this helper is expected to work correctly with standard GitHub URLs, so maybe it should be renamed to WithURLs and have the implementation fixed?

[gmlewis]

I'm finding this description a bit vague and hard to understand. Can you please give some concrete examples for:

• how it doesn't function as expected
• how it doesn't appear to support GitHub Enterprise Cloud
• what "slashes at either end" of a URL path means

[stevehipwell]

@gmlewis sorry, I should have been clearer. Now I've has a chance to go back over the code that I was refactoring I've noticed that it's not as black and white as I'd assumed (I should have read the code better and been in less of a rush).

Firstly I was working on an incorrect assumption about GHEC, the examples I was provided didn't have the api. prefix which would be required for the code to work but I think this was just a communication issue. The current code should work correctly.

Secondly this issue only happens when instead of using u = u.JoinPath() you use u.Path = path.Join() which strips the trailing / and doesn't add the leading / if not explicitly defined. This results in a GHES API URL getting an additional /api/v3/ suffix appended. This is definitely a caller problem, but relaxing the accepted pattern (e.g. adding a leading / if missing the same as the trailing / is handled) would make this harder to hold wrong.

Out of interest is there a requirement to copy the client or could this be replaced with the option pattern? I'd be happy to open a PR to take a look at how this would work.

[gmlewis]

Out of interest is there a requirement to copy the client or could this be replaced with the option pattern? I'd be happy to open a PR to take a look at how this would work.

Yes, we need to support copying the client. I can't find the issue now, but that was a feature that was added so that large batch processing jobs could more easily be supported.

[stevehipwell]

@gmlewis I get there being a requirement to copy the client, but I'm not sure why it has to happen to configure non-standard URLs? It seems like two different concepts got coupled? Wouldn't a pattern like below offer more flexibility?

client := github.NewClient(httpClient, github.WithToken(""))

client2 := client.Copy(github.WithURLs("", ""), github.WithToken(""))

client3 := client2.Copy(github.WithToken(""))

[gmlewis]

@gmlewis I get there being a requirement to copy the client, but I'm not sure why it has to happen to configure non-standard URLs? It seems like two different concepts got coupled? Wouldn't a pattern like below offer more flexibility?

client := github.NewClient(httpClient, github.WithToken(""))

client2 := client.Copy(github.WithURLs("", ""), github.WithToken(""))

client3 := client2.Copy(github.WithToken(""))

@WillAbides led the redesign of our client creation in #2897 and #2904, and may have more insight or opinions about this which I would like to hear.

[stevehipwell]

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.

It looks like the options pattern was discounted due to requiring an error in the signature, which doesn't make sense as this is one of the main reasons for adopting this pattern over leaving the struct directly accessible. It also doesn't make sense given that the WithEnterpriseURLs has an error in it's signature. If backwards compatibility is important, which isn't usually the case here, this could be implemented as github.NewClientWithOptions().

If I want to make a new client with a custom URL and a token (client, err := github.NewClient(httpClient).WithAuthToken("").WithEnterpriseURLs("", "")) I'm creating 3 clients just to get a single one.

[kyungseopk1m]

I'd like to take this if nobody else is on it.

The slash-handling fix is straightforward. For the rename, do you want to batch it with the v86 queue (alongside #4117 / #3644), or land the fix on v85 first and do the rename separately?

[gmlewis]

@stevehipwell - are you currently working on this, or shall I give this issue to @kyungseopk1m ?

[stevehipwell]

@gmlewis I'm currently working on this, will be opening a PR after #4151 is merged.

[gmlewis]

I'd like to take this if nobody else is on it.

Sorry, @kyungseopk1m, but @stevehipwell is already working on this one and I had neglected to assign the owner properly.

[kyungseopk1m]

Looks like this is already in progress, so I'll step back on this one. Thanks for the clarification.