Repository navigation
Client.RateLimits shouldn't return RateLimitError聽#1899
Description
馃憢 Hello,
I noticed that Client.RateLimits can return a RateLimitError, as per:
Lines 648 to 652 in 75644ea
According to GitHub REST API documentation, rate limit status can be checked "at any time using the Rate Limit API", i.e. even when the remaining number of allowed requests is 0, and the GET /rate_limit endpoint never responds with a HTTP 403. I verified myself and I can confirm the documentation is accurate. Therefore, I think Client.RateLimits should never return RateLimitError errors and return the actual rate limit status, by performing a HTTP request.
I have 2 solutions in mind:
Client.checkRateLimitBeforeDois aware of some allowlist of endpoints, includingGET /rate_limits(and perhaps others I don't know yet about), for which it should always returnnil;- a higher layer passes a boolean which indicates that the request should not be subject to the pre-request rate limit check, possibly exposed to the user so one could hit GitHub API regardless of the internal rate limit status.
Happy to discuss any solution before I (or someone else) patch(es) the issue. Thanks!
Activity
gmlewis commentedon Jun 17, 2021
May I please ask what the actual problem is that you are trying to solve?
In other words, why is it a problem that
Client.RateLimits()returns aRateLimitError(which, by the way, should have all the information you need to avoid causing the next rate limit error... specifically theResettimestamp)?sa-spag commentedon Jun 18, 2021
RateLimitErroronly exposes the rate limit status for the request's category, i.e.coreforGET /rate_limits, but there are a bunch of other "resources" for which rate limit is independent and for which I would like to request an up-to-date status.gmlewis commentedon Jun 18, 2021
I suppose a third option is to modify
Client.RateLimits()to always perform the call regardless of saved history by bypassing the existing check.sa-spag commentedon Jun 22, 2021
Thanks for your feedback. Exactly, and I think we have at least 2 ways of implementing such bypass mechanism (the 2 options I previously listed). Do not hesitate to provide any guidance to take a decision.
gmlewis commentedon Jun 22, 2021
My current thought is to not add to the complexity of
Client.checkRateLimitBeforeDo()but instead modify onlyClient.RateLimits()as necessary to implement this change.I'm fine for you to go ahead and create a PR if you are interested, @sa-spag, and let's see what it looks like.
Enable `Client.RateLimits` to bypass the rate limit check
Enable `Client.RateLimits` to bypass the rate limit check (#1907)
Enable `Client.RateLimits` to bypass the rate limit check (google#1907)