Skip to content
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

Update RESTClient range paginators to support stopping after empty pages #1637

Closed
timoguin opened this issue Jul 24, 2024 · 3 comments · Fixed by #1677
Closed

Update RESTClient range paginators to support stopping after empty pages #1637

timoguin opened this issue Jul 24, 2024 · 3 comments · Fixed by #1677
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@timoguin
Copy link

Feature description

Update RESTClient's PageNumberPaginator to optionally stop paging after receiving empty pages.

I'm unsure if other paginators need this same functionality. Perhaps it needs to be added to the base RangePaginator class.

Are you a dlt user?

Yes, I'm already a dlt user.

Use case

I am pulling data from a few REST APIs that do not include the information necessary to determine the total number of records or pages. Setting the max number of pages isn't sufficient because the REST helper will keep sending requests up to the max page limit, even if everything past the first page is empty.

Proposed solution

Add a boolean keyword argument to the PageNumberPaginator (or its base class RangePaginator) called stop_after_empty that is False by default.

When it is true, consider the paging complete when the first empty page is received.

Related issues

No response

@willi-mueller
Copy link
Collaborator

willi-mueller commented Aug 9, 2024

Thank you for your feature request @timoguin!
In the specification, you wrote that the default for this new flag should be False. I wonder whether defaulting to True would be more practical.

Have you encountered a case where a page delivers empty results but the next page is not empty?

What do you think @burnash?

If you like, please review this implementation: #1677

@burnash
Copy link
Collaborator

burnash commented Aug 9, 2024

Agree with @willi-mueller, practically it makes sense to stop after an empty page by default. I'm curious about your experience @timoguin

@timoguin
Copy link
Author

timoguin commented Aug 9, 2024

@burnash @willi-mueller I only proposed setting it to False to retain the current default behavior, but I agree that it makes sense to stop after empty pages by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants