Open Cidan opened 2 years ago
This CL is a WIP -- while all current tests pass and the changes are backwards compatible, there are no tests for the new functionality yet.
Okay, it turns out the base auth methods are tested because I moved around some of the token verification code to use the new auth options. PTAL!
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
---|---|---|---|---|---|
helix.go | 33 | 34 | 97.06% | ||
entitlement_codes.go | 2 | 4 | 50.0% | ||
extension_secrets.go | 0 | 4 | 0.0% | ||
authentication.go | 40 | 50 | 80.0% | ||
<!-- | Total: | 199 | 216 | 92.13% | --> |
Files with Coverage Reduction | New Missed Lines | % | ||
---|---|---|---|---|
helix.go | 1 | 89.74% | ||
<!-- | Total: | 1 | --> |
Totals | |
---|---|
Change from base Build 1364869930: | -0.6% |
Covered Lines: | 1245 |
Relevant Lines: | 1354 |
Looks good @Cidan! We're going to need to add some additional documentation for this change. Did you want to take a stab at that in this PR or in a separate PR?
Any update on this? I just realised this library is not for concurrent use because of this, 2 validate token requests cannot be made at the same time for example.
@Cidan I don't suppose you can start adding ctx
for first argument and pass it over to the request object? If the function APIs are being broken best to have it break once :+1:
Hi folks,
Sorry, I've been taking time off for the holidays here in the US. I'll finish this up next week when I return.
@bweston92 Currently, the API is backwards compatible -- this is not a breaking change. I don't want to add a context, as this would indeed make it a breaking change.
Oh I just seen you're doing variadic argument for the options. However you're only using the first one in the slice. That might not be very user friendly. Can I suggest we do something else, where we have an Option
type so the API would look like the following:
type Options struct {
ClientID string
ClientSecret string
AppAccessToken string
UserAccessToken string
UserAgent string
RedirectURI string
HTTPClient HTTPClient
RateLimitFunc RateLimitFunc
APIBaseURL string
ExtensionOpts ExtensionOptions
}
type Option func(o *Options) error
func WithClientID(clientID string) Option {
return func(o *Options) error {
o.ClientID = clientID
return nil
}
}
Then we can also add one WithTimeout
which would look like.
func WithTimeout(readTimeout time.Duration) Option {
return func(o *Options) error {
if ok, c := o.HTTPClient.(*http.Client); ok {
c.Timeout = readTimeout
}
return nil
}
}
I'm open to introducing breaking changes. We'll just need to bump to v3.
I'm also ok with the variadic functions approach. Variadic functions are used widely throughout the Go standard library which is a good point of reference.
@Cidan I've just merged #133. Could you please make sure to update GetExtensionLiveChannels()
to a variadic function as part of this PR too? Thanks :slightly_smiling_face:
I would take consideration with the Option
type rather then passing a Options
struct as a slice and only using the first occurrence. I'm guessing it was done as a hack job.
If you're willing to have breaking changes then add a context too.
I'd be willing to send a PR to @Cidan's fork if they're welcoming of that change.
I'm absolutely supportive of that change @bweston92 -- you're right that it was added quickly. I wanted to make minimal changes, and at the time it seemed like the easiest path. Feel free to send the PR to my fork, and I'll merge it, along with context if you'd like.
Sorry, one more thing @bweston92; my only concern with your outlined approach is that it's done correctly in the context of this client. If we're going to implement the whole WithXXX option setters, we should do it correctly, as seen in gRPC DialOptions for example.
This CL adds an optional parameter to all API calls that allows the user to specific authentication parameters on a per-call basis. When authentication parameters are passed in, the authentication parameters in the client it self are ignored.
The rational for this change is described in #128 -- without the ability to handle auth state outside of the client, a user must instantiate and keep track of multiple copies of a client to achieve a high degree of parallelism using this library.