Permit toggling rate limit check by consumers #3386
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3386 +/- ##
==========================================
- Coverage 97.72% 92.21% -5.51%
==========================================
Files 153 173 +20
Lines 13390 14770 +1380
==========================================
+ Hits 13085 13620 +535
- Misses 215 1060 +845
Partials 90 90 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you, @manicminer !
Right before line 804 in github/github.go, could you please add a comment that demonstrates how an end-user would override this feature in their own code?
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
I'm sorry, I got confused |
|
@DiegoDev2 Thanks for the review! I've added a go doc comment for this constant. |
There was a problem hiding this comment.
Thank you, @manicminer !
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
| // BypassRateLimitCheck prevents a pre-emptive check for exceeded primary rate limits | ||
| // Specify this by providing a context with this key, e.g. | ||
| // context.WithValue(context.Background(), github.BypassRateLimitCheck, true) | ||
| BypassRateLimitCheck requestContext = iota |
There was a problem hiding this comment.
Why is BypassRateLimitCheck exported? Can't it remain bypassRateLimitCheck?
There was a problem hiding this comment.
Oh I missed that you wanted to trigger this from outside the library.
I suggest that there will be wrapper functions like:
func ContextWithBypassRateLimitCheck(ctx context.Context) context.Context {
return context.WithValue(ctx, bypassRateLimitCheck, struct{}{})
}You don't even need to export the getter function for this value.
Anyway, as the code is now is fine
| // BypassRateLimitCheck prevents a pre-emptive check for exceeded primary rate limits | ||
| // Specify this by providing a context with this key, e.g. | ||
| // context.WithValue(context.Background(), github.BypassRateLimitCheck, true) | ||
| BypassRateLimitCheck requestContext = iota |
There was a problem hiding this comment.
Oh I missed that you wanted to trigger this from outside the library.
I suggest that there will be wrapper functions like:
func ContextWithBypassRateLimitCheck(ctx context.Context) context.Context {
return context.WithValue(ctx, bypassRateLimitCheck, struct{}{})
}You don't even need to export the getter function for this value.
Anyway, as the code is now is fine
Export
BypassRateLimitCheckto allow toggling of rate limit checks by consumers.I am handling rate-limited retries with my own RoundTripper, and the built-in checks are preventing me from doing so because it simply errors early instead of attempting a request and handling backoff separately. This change simply exports the context key used internally by the package, so that downstream consumers can disable this pre-emptive check.