yanyongyu / githubkit

The modern, all-batteries-included GitHub SDK for Python, including rest api, graphql, webhooks, like octokit!
MIT License
158 stars 21 forks source link

Feature: support detecting rate limit exceeded #80

Closed schelv closed 6 months ago

schelv commented 6 months ago

I've added a RateLimitExceededError class. This exception provides the time you have to wait before retrying, which simplifies implementing rate limiting.

codecov[bot] commented 6 months ago

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (8a381cb) 75.60% compared to head (3b8ee2a) 35.22%. Report is 2 commits behind head on master.

Files Patch % Lines
githubkit/core.py 28.57% 20 Missing :warning:
githubkit/exception.py 71.42% 2 Missing :warning:
githubkit/graphql/__init__.py 50.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #80 +/- ## =========================================== - Coverage 75.60% 35.22% -40.39% =========================================== Files 541 2414 +1873 Lines 59462 124872 +65410 =========================================== - Hits 44957 43987 -970 - Misses 14505 80885 +66380 ``` | [Flag](https://app.codecov.io/gh/yanyongyu/githubkit/pull/80/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ju4tCode) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/yanyongyu/githubkit/pull/80/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ju4tCode) | `35.22% <40.00%> (-40.39%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ju4tCode#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

yanyongyu commented 6 months ago

Some references may be helpful, take a note here:

https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28#exceeding-the-rate-limit

https://docs.github.com/en/graphql/overview/rate-limits-and-node-limits-for-the-graphql-api#exceeding-the-rate-limit

https://github.com/octokit/plugin-throttling.js/blob/135a0f556752a6c4c0ed3b2798bb58e228cd179a/src/index.ts#L134-L179

yanyongyu commented 6 months ago

@schelv i have made some changes to make the handle clearly. Could you please re-check this to ensure the handle is corrent? Thanks a lot.

yanyongyu commented 6 months ago

It seems the graphql rate limit is not handled.

schelv commented 6 months ago

It seems the graphql rate limit is not handled.

They give a HTTP 200 status code when you exceed that limit 🤦‍♂️ https://docs.github.com/en/graphql/overview/rate-limits-and-node-limits-for-the-graphql-api#exceeding-the-rate-limit

Maybe this? 😄

        if response.is_error:
            # check for errors.
            ....
        else:
            # also check for errors.
            ...
yanyongyu commented 6 months ago

For graphql, according to the octokit throttling plugin, an error with RATE_LIMITED type will be returned when primary rate limit exceeded. i'm not sure if this will cover all cases (secondary rate limit). this may be improved in the feature.

schelv commented 6 months ago

Looks good! What to do with the checks that are failing?

yanyongyu commented 6 months ago

Don't worry about the reduced coverage, this is an imperfect test and is only used to ensure that it can run.