-
Notifications
You must be signed in to change notification settings - Fork 75
Fixing 403 error always being RateLimiting #685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| return any(header.lower() in [h.lower() for h in headers] for header in RATE_HEADERS) | ||
| # Check if rate limit is actually exhausted (remaining requests is 0) | ||
| headers_lower = {k.lower(): v for k, v in headers.items()} | ||
| remaining = headers_lower.get("x-ratelimit-remaining") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x-ratelimit-remaining is probably not universal (?). Quick search for equivalent headers could help us make this more robust ratelimit-remaining, x-rate-limit-remaining, x-app-rate-limit-remaining, etc.
| # Check if rate limit is actually exhausted (remaining requests is 0) | ||
| headers_lower = {k.lower(): v for k, v in headers.items()} | ||
| remaining = headers_lower.get("x-ratelimit-remaining") | ||
| return remaining is not None and remaining.isdigit() and int(remaining) == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the value isn't a digit then are there other common formats that we could handle here?
Reponse 403 was returning RateLimiting all the time, but it was due only checking if rate limiting header exists, but it should be checked if it is 0 also.