refine pagination code, other minor refinements #140
Conversation
…s Search result, rename OpportunityRequest to OpportunityPayload
philvarner
changed the title
| self.pagination_link(request, search.model_dump(mode="json")) | ||
| ) | ||
| links.append(self.order_link(request, search)) | ||
| links.append(self.pagination_link(request, search, pagination_token)) |
There was a problem hiding this comment.
this pushes the logic for how to construct the link down into the pagination_link method
| ): | ||
| case Success(order): | ||
| self.root_router.add_order_links(order, request) | ||
| order.links.extend(self.root_router.order_links(order, request)) |
There was a problem hiding this comment.
improves the semantics of the function to make it more functional -- e.g., it doesn't mutate it's parameters, and instead just returns an object that the caller can then use for mutation
| body["next"] = pagination_token | ||
| return Link( | ||
| href=str(request.url.remove_query_params(keys=["next", "limit"])), | ||
| href=str(request.url), |
There was a problem hiding this comment.
there won't be query params on this, so they don't need to be removed
| def pagination_link(self, request: Request, pagination_token: str, limit: int): | ||
| return Link( | ||
| href=str(request.url.include_query_params(next=pagination_token)), | ||
| href=str( |
There was a problem hiding this comment.
include the limit explicitly, so it's not just relying on whatever the default is
| next_token = next_token.split("next=")[1] | ||
| params = {"next": next_token, "limit": limit} | ||
| res = stapi_client.get(endpoint, params=params) | ||
| o = urlparse(url) |
There was a problem hiding this comment.
re-wrote this to actually parse the url, since a simple split on next= doesn't work if there are any other parameters, e.g., next=foo&limit=1, since the next_token value gets set to foo&limit=1
There was a problem hiding this comment.
ahh shoot wish I had written the tests that would've caught that case in the first place
| order["links"].append(json_link) | ||
| self_link = copy.deepcopy(order["links"][0]) | ||
| order["links"].append(self_link) | ||
| monitor_link = copy.deepcopy(order["links"][0]) |
There was a problem hiding this comment.
this was missing from the /orders endpoint Orders and was added, so the test was updated
philvarner
changed the title
theodorehreuter
left a comment
theodorehreuter
left a comment
There was a problem hiding this comment.
Good stuff, nice improvements and tidying.
| def search_body(self) -> dict[str, Any]: | ||
| return self.model_dump(mode="json", include={"datetime", "geometry", "filter"}) | ||
|
|
||
| def body(self) -> dict[str, Any]: |
There was a problem hiding this comment.
I really like the addition of these small helper functions here to help clean up the endpoint business logic.
| body["next"] = pagination_token | ||
| return Link( | ||
| href=str(request.url.remove_query_params(keys=["next", "limit"])), | ||
| href=str(request.url), |
| next_token = next_token.split("next=")[1] | ||
| params = {"next": next_token, "limit": limit} | ||
| res = stapi_client.get(endpoint, params=params) | ||
| o = urlparse(url) |
There was a problem hiding this comment.
ahh shoot wish I had written the tests that would've caught that case in the first place
| res = stapi_client.get(endpoint, params=params) | ||
| o = urlparse(url) | ||
| base_url = f"{o.scheme}://{o.netloc}{o.path}" | ||
| parsed_qs = parse_qs(o.query) |
There was a problem hiding this comment.
didn't know about this parse_qs function, that would've been helpful early only womp womp. The more you know.
Related Issue(s):
Proposed Changes:
PR Checklist: