Skip to content
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

feat: add retry #51

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: add retry #51

wants to merge 6 commits into from

Conversation

thejuan
Copy link

@thejuan thejuan commented May 3, 2021

Airtable rate limits to 5 requests per second across all bases. They return http code 429 when you hit this limit.
This PR introduces retries via the requests retry functionality with back off so this error can be avoided.

I think this is a better approach than local rate limiting as there maybe multiple clients accessing the API, and is more aligned with the official node library


This change is Reviewable

Copy link
Owner

@pcorpet pcorpet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your suggestion: I've run into 429 myself several times and your change will be useful.

Can you address the styling comments + the use of requests_mock please? Otherwise I'll do the cleanup.

Reviewable status: 0 of 4 files reviewed, 7 unresolved discussions (waiting on @thejuan)


.gitignore, line 11 at r1 (raw file):

/ChangeLog
/.eggs
/.vscode

Please keep this in your own .gitignore


test_airtable.py, line 5 at r1 (raw file):

import requests_mock
import httpretty

Please use requests_mock lib to mock response instead of adding a new library.


test_airtable.py, line 52 at r1 (raw file):

                ),
                 httpretty.Response(
                    body='{}',

I suggest you add an actual response and that you check below that it was indeed return.


test_airtable.py, line 57 at r1 (raw file):

            ]
        )
        

Please clean trailing whitespaces.


test_airtable.py, line 60 at r1 (raw file):

        self.get()
        self.assertEqual(3, len(httpretty.latest_requests()))
       

Let's keep only one blank line between methods please.


airtable/init.py, line 8 at r1 (raw file):

import requests
from requests.packages.urllib3.util.retry import Retry

To be consistent I prefer if we only import modules (except for types). So here import retry and then below use retry.Retry


airtable/init.py, line 95 at r1 (raw file):

        self._dict_class = dict_class

    

Please drop this line (only one blank line between methods)

@thejuan
Copy link
Author

thejuan commented May 5, 2021

Please use requests_mock lib to mock response instead of adding a new library.

Unfortunately requests_mock uses a custom adapter, which replaces the retry adapter. It can't be used to test adapter features.

Will fix up the rest, do you use a formatter for these I can run?

@thejuan thejuan requested a review from pcorpet May 5, 2021 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants