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. |
gmlewis
left a comment
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. |
gmlewis
left a comment
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
|
Thanks for the reviews! Could this be merged? It looks like the codecov check is a false negative / unrelated to this PR?
|
|
Thank you, @tomfeigin and @manicminer ! |
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.