-
Notifications
You must be signed in to change notification settings - Fork 990
refactor: extract pagination logic into shared helper function #1770
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
Conversation
@booxter FYI. |
If we agree on the approach, I'll update all the API lists with this. |
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 is a good step. Ultimately, I'd try to apply this logic automatically with a class factory / ABCMeta, to make sure all GETs that return list responses are covered by it without explicitly requesting the behavior with return paginate()
.
data=rows, | ||
next_start_index=end if end < len(loaded_dataset) else None, | ||
) | ||
records = [loaded_dataset[i] for i in range(len(loaded_dataset))] |
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.
it seems redundant now that you paginate as the last step that receives all records.
a10f62d
to
21a59b6
Compare
Move pagination logic from LocalFS and HuggingFace implementations into a common helper function to ensure consistent pagination behavior across providers. This reduces code duplication and centralizes pagination logic in one place. Signed-off-by: Sébastien Han <[email protected]>
""" | ||
|
||
data: List[Dict[str, Any]] | ||
has_more: bool |
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.
can you add a todo to add the url to fetch more?
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.
Sure, I'll open a ticket for this.
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 PR do?
Move pagination logic from LocalFS and HuggingFace implementations into a common helper function to ensure consistent pagination behavior across providers. This reduces code duplication and centralizes pagination logic in one place.
Test Plan
Run this script:
And play with
start_index
andlimit
.