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

Add pagination example using cursors #93

Merged

Conversation

ymdpharm
Copy link
Contributor

Hi, thanks for a nice product.

I found it inconvenient that XRPC's get_follows and get_followers are limited to 100 results at a time. So I've added some methods on client to address this issue. I would like to commit the changes to help someone.

There doesn't seem to be test cases yet, but I've verified the behavior.

@ymdpharm ymdpharm changed the title Feature/add get all follows followers add get_all_follow(er)s methods Jul 17, 2023
@MarshalX
Copy link
Owner

MarshalX commented Jul 17, 2023

Hi! Thank you for your contribution!

That's a simple pagination that is presented in many methods. Not only in getting followers. I see 2 ways how we can improve understanding of it:

  1. Make utils to work with paginations. Doesn't matter which method. It must work with all that supports cursor arg.
  2. Move your method with a while loop to examples instead of methods of the clients

What do you think?

@ymdpharm
Copy link
Contributor Author

I think 1 is better. Pagination should be hidden from users.

There are some issues, one of them is that it would be better to define the return value signature (Response type) on our own to achieve 1.

In this commit, I aimed to quickly provide what the users want, expecting future refactoring.

@MarshalX
Copy link
Owner

MarshalX commented Jul 18, 2023

to be honest I really don't want to make hundreds of requests under the hood without the user's understanding. also, I don't want to have to copy and paste for every method that supports the "cursor" param. we need something that will work for all

JUST A PSEUDO CODE THOUGHTS:

def get_all_paginated_entries(callable, *args, **kwargs) -> BaseResponseModel:
    # we will get the return type annotation of callable in runtime and will perform checks that the response contains cursor attr
    # we will get the params data type annotation of callable in runtime and will perform checks that params contains cursor attr
    # we will implement universal loop do deal with cursor
    # we will document this method that it could lead to long execution time and many http request
    ...

Example of usage:

all_followers = get_all_paginated_entries(client.bsky.graph.get_follows, actor='marshal.dev)

using of get_all_paginated_entries method will be clear in the code that it will lead to long execution and it's user's choice

the list of methods with "cursor" is huge:
image

@ymdpharm
Copy link
Contributor Author

I really don't want to make hundreds of requests under the hood without the user's understanding.

I understood. I think it also makes sense.
I've moved codes to examples. It may help someone.

I may suggest some solutions next time. Thanks in advance then.

Copy link
Owner

@MarshalX MarshalX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! here are a few notes

examples/advanced_usage/handle_cursor_pagination.py Outdated Show resolved Hide resolved
examples/advanced_usage/handle_cursor_pagination.py Outdated Show resolved Hide resolved
@MarshalX MarshalX changed the title add get_all_follow(er)s methods Add pagination example using cursors Jul 18, 2023
@MarshalX MarshalX merged commit f870a80 into MarshalX:main Jul 18, 2023
3 checks passed
@MarshalX
Copy link
Owner

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants