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 warning to paginated request that was performed by req_perform() and not paginate_req_perform() #336

Closed
asadow opened this issue Oct 10, 2023 · 4 comments

Comments

@asadow
Copy link

asadow commented Oct 10, 2023

I was scratching my head for a while wondering why pagination wasn't working until I realized I had used req_perform() and not paginate_req_perform() .

A warning to a paginated request performed via req_perform() might be appropriate to signal that the pagination was ignored. Alternatively, is it possible to somehow integrate pagination into req_perform() and obviate the need for paginate_req_perform()?

@asadow
Copy link
Author

asadow commented Oct 10, 2023

I see that this was already suggested by @jonthegeek with the below reply

To inform in req_perform() sounds nice but this would also imply an extra argument to req_perform() to switch off the information in case you actually want to use req_perform(). For now I simply pointed to paginate_req_perform() more clearly in the documentation

Originally posted by @mgirlich in #279 (comment)

@mgirlich
Copy link
Collaborator

I agree that it would be nice to help the user here a little more.

Alternatively, is it possible to somehow integrate pagination into req_perform() and obviate the need for paginate_req_perform()?

I'm not convinced integrating paginate_req_perform() into req_perform() is a good idea. We definitely need two functions: 1) a function that performs the request for a single page, and 2) a function that performs the pagination.
So, the question is whether we add a new function for 1 or for 2. I guess we gain more flexibility by adding a function for 2 (e.g. an error policy, a progress bar, and limiting the number of requests).

Some ideas what we could do if we add a warning to req_perform() when applied to a paginated request:

  1. Allow to switch off warning via an argument.
  2. Add req_perform_page() (this would need a better name) to request a single page.
  3. Add remove_pagination().

@hadley Do you have any good ideas?

@hadley
Copy link
Member

hadley commented Oct 11, 2023

None of the options seem particularly appealing to me, so I'd prefer to stick to the status quo until we come up with something better.

@hadley
Copy link
Member

hadley commented Oct 12, 2023

Now that we have the req_perform() family of functions (in #335), I'm leaning more strongly to closing this for now. Lets give the dust some time to settle before we make more changes.

@hadley hadley closed this as completed Oct 12, 2023
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

No branches or pull requests

3 participants