-
Notifications
You must be signed in to change notification settings - Fork 846
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
Improve github paginator #2823
Improve github paginator #2823
Conversation
Signed-off-by: Andrew Brain <[email protected]>
Signed-off-by: Andrew Brain <[email protected]>
Signed-off-by: Andrew Brain <[email protected]>
Signed-off-by: Andrew Brain <[email protected]>
@@ -4,6 +4,7 @@ | |||
from augur.tasks.init.celery_app import celery_app as celery | |||
from augur.tasks.init.celery_app import AugurCoreRepoCollectionTask, AugurSecondaryRepoCollectionTask | |||
from augur.application.db.data_parse import * |
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.
[pylint] reported by reviewdog 🐶
W0401: Wildcard import augur.application.db.data_parse (wildcard-import)
|
||
return response.json() | ||
|
||
# TODO: Handle timeout exceptions better |
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.
[pylint] reported by reviewdog 🐶
W0511: TODO: Handle timeout exceptions better (fixme)
@@ -0,0 +1,185 @@ | |||
import logging |
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.
[pylint] reported by reviewdog 🐶
C0114: Missing module docstring (missing-module-docstring)
from urllib.parse import urlparse, parse_qs, urlencode | ||
|
||
|
||
class RatelimitException(Exception): |
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.
[pylint] reported by reviewdog 🐶
C0115: Missing class docstring (missing-class-docstring)
class RatelimitException(Exception): | ||
pass | ||
|
||
class UrlNotFoundException(Exception): |
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.
[pylint] reported by reviewdog 🐶
C0115: Missing class docstring (missing-class-docstring)
class UrlNotFoundException(Exception): | ||
pass | ||
|
||
class GithubDataAccess: |
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.
[pylint] reported by reviewdog 🐶
C0115: Missing class docstring (missing-class-docstring)
|
||
return (100 * (num_pages -1)) + len(data) | ||
|
||
def paginate_resource(self, url): |
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.
[pylint] reported by reviewdog 🐶
R1711: Useless return at end of function or method (useless-return)
|
||
return int(parse_qs(parsed_url.query)['page'][0]) | ||
except (KeyError, ValueError): | ||
raise Exception(f"Unable to parse 'last' url from response: {response.links['last']}") |
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.
[pylint] reported by reviewdog 🐶
W0707: Consider explicitly re-raising using 'except (KeyError, ValueError) as exc' and 'raise Exception(f"Unable to parse 'last' url from response: {response.links['last']}") from exc' (raise-missing-from)
|
||
response = client.request(method=method, url=url, timeout=timeout, follow_redirects=True) | ||
|
||
if response.status_code in [403, 429]: |
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.
[pylint] reported by reviewdog 🐶
R1720: Unnecessary "elif" after "raise", remove the leading "el" from "elif" (no-else-raise)
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.
Ready to test!
Description
response.raise_for_status()
httpx method to ensure that an exception is thrown if a failure status code is returned (this allows us to assume that the response is successful if no exceptions occur Also themake_request()
method throws a rate limit exception so that the caller can determine how to handle the rate limit, but the higher level methods like paginate resource and get resource handle the rate limit like the github api docs tell us to.I did this work to support the core recollection work, since I need a reliable method to get the page count of a resource and the old github paginator was not reliable enough.
Currently I only added it to the pull request task, but over time I will start to add it to other tasks as we become more confident in it
Signed commits