-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
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.
Nice! Few thoughts lmk what you think
parent_stream_type = RepositoryStream | ||
ignore_parent_replication_key = False | ||
state_partitioning_keys = ["repo", "org"] | ||
tolerated_http_errors = [404] |
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.
this might be a good thing to add to the README too since a project could technically not have a readme, and it would halt here. Good catch!
self, prepared_request, context: Optional[dict] | ||
) -> requests.Response: | ||
"""Override private method _request_with_backoff to account for expected 404 Not Found erros.""" | ||
# TODO - Adapt Singer |
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.
what does this mean?
def _request_with_backoff( | ||
self, prepared_request, context: Optional[dict] | ||
) -> requests.Response: | ||
"""Override private method _request_with_backoff to account for expected 404 Not Found erros.""" |
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.
Typo @ erros
tap_github/client.py
Outdated
extra_tags=extra_tags, | ||
) | ||
if response.status_code in self.tolerated_http_errors: | ||
self.logger.info("Request returned a tolerated error for {}".format(prepared_request.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.
f strings are supported right? Prefer those since they are a lot nicer.
return response | ||
|
||
if response.status_code in [401, 403]: | ||
self.logger.info("Failed request for {}".format(prepared_request.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.
f strings here too
def parse_response(self, response: requests.Response) -> Iterable[dict]: | ||
"""Parse the response and return an iterator of result rows.""" | ||
# TODO - Split into handle_reponse and parse_response. |
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.
TODO reminder
@@ -71,13 +72,61 @@ def get_url_params( | |||
params["since"] = since | |||
return params | |||
|
|||
def _request_with_backoff( |
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.
We never call the super (from what I see) which I'm guessing means we copy a lot of logic from the parent. Probably better to try change the underlying impl via a fork or message to the maintainer? Or maybe we can call the super and add extra code as needed.
@Ry-DS for some reason I had not seen these remarks. It's been merge on the parent main already, but feel free to open a new PR to make some improvements! |
No description provided.