Handle GitHub API rate limits #3366
Conversation
martinhoyer
added
the
code | utils
martinhoyer
requested review from
psss,
lukaszachy,
happz,
thrix and
janhavlin
as code owners
tmt/utils/__init__.py
Outdated
| @@ -4358,6 +4358,33 @@ def increment( | |||
|
|
|||
| raise GeneralError(message) from error | |||
|
|
|||
| # Handle GitHub-specific responses | |||
| # https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28#exceeding-the-rate-limit | |||
| response = kwargs.get('response') | |||
There was a problem hiding this comment.
is it possible to use cast() to introduce some types? See error above, it should be possible to do something like response = cast(Optional[requests.Response], kwargs.get('response')). The same applies to headers below, headers: dict[str, str] should help linters & limit the amount of Any running around.
There was a problem hiding this comment.
And maybe headers we would get for free, response.headers should exist all the time, even if it'd be empty, i.e. no need for getattr?
There was a problem hiding this comment.
@happz Thanks, adding cast already found an issue :)
I'm not sure about the headers, as that already has CaseInsensitiveDict[str] type.
There was a problem hiding this comment.
Yes, once linters become aware of the response type, I suppose they would get a better view of its attributes, and we wouldn't need to bother with cast calls.
There was a problem hiding this comment.
hmm, they are already aware? it was pre-commit mypy that caught that when I tried to do dict[str]
There was a problem hiding this comment.
I would expect kwargs.get('response') returns Any, because of **kwargs: Any. As soon as cast(requests.Response, ...) is applied, mypy should be able to infer the rest, but without this hint, it'd all be just Any.
|
/packit test |
| @@ -4358,6 +4358,30 @@ def increment( | |||
|
|
|||
| raise GeneralError(message) from error | |||
|
|
|||
| # Handle GitHub-specific responses | |||
| # https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28#exceeding-the-rate-limit | |||
| response = cast(requests.Response, kwargs.get('response')) | |||
There was a problem hiding this comment.
Must be Optional, get() may return None.
As per discussion on chat. Trying to handle rate limits for GitHub api.
https://stackoverflow.com/questions/66522261/does-github-rate-limit-access-to-public-raw-files
Pull Request Checklist