NewMode retries and pagination #1499
Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
|
@codybraun feel free to @ me once you're done adding things. also hope the linting and formatting checks aren't too burdensome - please let me know if they are! |
@shaunagm I'm done! The CI checks are all very reasonable, I've just been too lazy to configure VSC with the different linting setup for Parsons, which is on me! I'll do that if I'm committing code regularly here |
shaunagm
left a comment
shaunagm
left a comment
There was a problem hiding this comment.
So I overall support this but I'm not sure I understand how base_request, paginated_request and converted_request relate to each other
| logger.error("Response is not in JSON format.") | ||
| raise ValueError(f"API request encountered an error. Response: {response}") | ||
|
|
||
| def base_request( |
There was a problem hiding this comment.
did a couple of these params on base_request get removed (endpoint, supports_version, etc)? what's the reasoning there?
There was a problem hiding this comment.
yeah we were previously building the full URL in base_request- that's fine, except now we need some method that can take the already-full-url returned by "next". So we need a base method that doesn't do any decorating of the URL- the same thing should still be happening it is just a level higher in the paginator method
| if params is None: | ||
| params = {} | ||
|
|
||
| for attempt in range(retries + 1): |
There was a problem hiding this comment.
Why retries +1? and not just retries?
There was a problem hiding this comment.
that just keeps this sort of semantically correct- so if you said retries=0, you probably still want it to attempt once and then fail. retries=1 should mean two total attempts, etc. - make sense?
| raise e | ||
| raise Exception(f"Failed to retrieve data from {url} after {retries} attempts.") | ||
|
|
||
| def paginate_request( |
There was a problem hiding this comment.
Oh is this where those other params ended up? Can you briefly at a high level explain the overall design/refactoring happening here?
There was a problem hiding this comment.
Yep, basically we need somewhat of a refactor to handle the full URL returned by the "next" pointer to handle iteration. That next pointer already includes the API version, the endpoint, etc. so we want some method that just takes the URL and makes the request, then does our status checks and returns if those look good.
That just means when we are first assembling the URL for an initial request, we just need to do that a level higher (the new pagination wrapper method). Does that make sense? Open to other suggestions for how to refactor this but I don't see where we could handle the pagination without some changes
There was a problem hiding this comment.
and converted_request I didn't really mess with in part because I don't feel like I have a great understanding of Parsons tables- but basically that wraps paginate_request which wraps base_request
There was a problem hiding this comment.
No need to change - this makes perfect sense, I just couldn't quite follow the logic (the type hints while a nice addition made it a little harder to parse what substantively was being changed)
There was a problem hiding this comment.
yeah sorry, the typing added a bunch to the diff
| params = {"action": campaign_id} | ||
| response = self.converted_request(endpoint="submission", method="GET", params=params) | ||
| response = self.converted_request( | ||
| endpoint="submission", method="GET", params=params, data_key=RESPONSE_DATA_KEY |
There was a problem hiding this comment.
I am curious why some calls to converted_request pass in data_key=RESPONSE_DATA_KEY and some don't
There was a problem hiding this comment.
yeah I tried to avoid changing this as much as I could, but it looks like that's because in some places we're GETting campaigns/ which returns something like {"data": [{"campaign_id":1....}] and in other we're GETing /campaigns/1 which returns {"campaign_id":1....} (note that this is not nested)
shaunagm
added
the
breaking change
|
I'm adding the breaking change label just in case so I can flag it in the release notes, even though the signature we're changing is a private method and so it's not a big deal that we're breaking it |
What is this change?
Considerations for discussion
How to test the changes (if needed)
pytest test(obviously you'll need to add client id and secret to this and pick a campaign id that exists in that account)