RESTClient: add PageNumberPaginator #1307
Conversation
✅ Deploy Preview for dlt-hub-docs canceled.
|
| @@ -64,7 +64,133 @@ def update_request(self, request: Request) -> None: | |||
| return | |||
|
|
|||
|
|
|||
| class OffsetPaginator(BasePaginator): | |||
| class BaseNumericPaginator(BasePaginator): | |||
There was a problem hiding this comment.
@rudolfix, not sure if the name should be "Base*", this class could be useful on its own. What do you think?
There was a problem hiding this comment.
Maybe Simple* is better for tbh both look fine
There was a problem hiding this comment.
heh I spent 10 minutes thinking about good name :) maybe RangePaginator? because it looks like python range
| @@ -64,7 +64,133 @@ def update_request(self, request: Request) -> None: | |||
| return | |||
|
|
|||
|
|
|||
| class OffsetPaginator(BasePaginator): | |||
| class BaseNumericPaginator(BasePaginator): | |||
There was a problem hiding this comment.
heh I spent 10 minutes thinking about good name :) maybe RangePaginator? because it looks like python range
| self, | ||
| param_name: str, | ||
| initial_value: int, | ||
| total_path: jsonpath.TJsonPath, |
There was a problem hiding this comment.
When writing a paginator for moviesdb some endpoints had a regular page number paginator, but others were a top list endpoints where the total was a huge number which could not be reached, so I did this
https://github.com/dlt-hub/dlt-rest-client-hackathon/blob/main/moviesdb/page_number_paginator.py#L17
Where I could put a maximum value on total, with maximum value the total is optional. does it make sense to add it? IMO those broken paginators are quite common.
in that case we could thing about base class as a real range(start, stop, step)
There was a problem hiding this comment.
@rudolfix thanks for suggestion. I think it could be very useful to control the pagination.
I've implemented the maximum_value in the f7f7860.
I also really like the idea of making maximum_value a substitute for total number – this way we make this paginator very versatile. Will send the update soon.
| self._has_next_page = False | ||
|
|
||
| def _handle_missing_total(self, response_json: Dict[str, Any]) -> None: | ||
| raise ValueError( |
There was a problem hiding this comment.
this is good! thx! I'll probably create specialized exceptions for pagintors in #1281
@sultaniman which two groups do you mean? TestOffsetPaginator & TestPageNumberPaginator? |
burnash
removed
the
sprint
There was a problem hiding this comment.
take a look at the comments, you can fix them and merge. the code overall is good.
what worries me is that we do not do any end to end tests for paginators. why not to extend mock server and check them out?
maybe you could create a ticket for that and @sultaniman / you / me can implement it later :)
| self.limit_param = limit_param | ||
| self.total_path = jsonpath.compile_path(total_path) | ||
| if initial_limit is not None: | ||
| warnings.warn( |
There was a problem hiding this comment.
we have really good system for deprecations, look for DltDeprecationWarning not only does this but also shows usage of such functions in mypy and VSCode :)
|
@burnash btw. docstrings are PRO |
|
@rudolfix sure I will create a note for myself to improve testing then. |
burnash
force-pushed
the
feat/rest_client_pagenumberpaginator
branch
from
efd9070 to
5dfb678
Compare
Implements #1205
This PR also extracts the common code from the
OffsetPaginatorintoBaseNumericPaginator.