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

Use common req_perform_ prefix #335

Merged
merged 13 commits into from
Oct 12, 2023
Merged

Use common req_perform_ prefix #335

merged 13 commits into from
Oct 12, 2023

Conversation

hadley
Copy link
Member

@hadley hadley commented Oct 9, 2023

Fixes #314

@hadley hadley requested a review from mgirlich October 9, 2023 18:37
R/multi-req.R Outdated
#' # you'll need to inspect the results to figure out which requests fails
#' fail <- vapply(resps, inherits, "error", FUN.VALUE = logical(1))
#' resps[fail]
multi_req_perform <- function(reqs, paths = NULL, pool = NULL, cancel_on_error = FALSE) {
req_perform_multi <- function(reqs, paths = NULL, pool = NULL, cancel_on_error = FALSE) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this should be req_perform_parallel()? At least this sounds a bit more intuitive to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah, I like that name.

@mgirlich
Copy link
Collaborator

mgirlich commented Oct 10, 2023

I'm not sure we want to stick with req_perform_paginate() in the end. If we add req_chunk() we would either have to add req_perform_chunk() or change req_chunk() to return a list of requests.
Or we change the name to something like req_perform_iterative() which would cover paginated and chunked requests.

@hadley
Copy link
Member Author

hadley commented Oct 11, 2023

How about req_perform_iterate()?

@hadley
Copy link
Member Author

hadley commented Oct 11, 2023

@mgirlich should we rename req_stream() to req_perform_stream()?

@hadley hadley mentioned this pull request Oct 11, 2023
@mgirlich
Copy link
Collaborator

I like req_perform_stream() and req_perform_iterate()!

R/multi-req.R Outdated Show resolved Hide resolved
R/paginate.R Outdated Show resolved Hide resolved
R/paginate.R Outdated Show resolved Hide resolved
R/paginate.R Outdated Show resolved Hide resolved
hadley and others added 5 commits October 12, 2023 16:17
@hadley
Copy link
Member Author

hadley commented Oct 12, 2023

I really like this section now. Feels like the functions fit together into a whole.

Screenshot 2023-10-12 at 16 30 47

@hadley
Copy link
Member Author

hadley commented Oct 12, 2023

This makes me lean towards making the next release 1.0.0 again.

@hadley
Copy link
Member Author

hadley commented Oct 12, 2023

I'm swinging towards req_perform_iteratively() since that would match req_perform_parallel() better — i.e. perform iteratively; perform in parallel.

Or possibly req_perform_sequential()? But that does lose the "next request is generated from current response" implication of iterative.

Maybe we need req_perform_sequential() as well though? i.e. if you have a list of requests and want to do them one at a time? (Although I'm not sure why you would want that.)

@mgirlich
Copy link
Collaborator

req_perform_iteratively() also sounds good and I prefer it over req_perform_sequential().

One use case that comes to my mind for req_perform_sequential() would be for requests that create something. E.g. consider GitHub: the first request creates a repository, the second request creates an issue in this repository, the third request creates a comment in this issue.

Harder to come up with something for GET requests. Basically the only things that comes to my mind is if you don't want to do too many requests (e.g. because they cost something) but you don't know whether you will need all of them.
But the most realistic case for something like this would probably be a paginated search endpoint, where you don't (really) know all the requests in advance.
This brings me to the idea that for this we might want to include a break callback to req_perform_iteratively() at some point.

@hadley
Copy link
Member Author

hadley commented Oct 12, 2023

I'll make the change to req_perform_iteratively() and then merge this PR. Let's continue to talk about the interface for iterative requests; I think if we can get this right, I'll feel happy making the next release 1.0.0 and it's worth taking some extra time to do that.

@hadley hadley merged commit c2559b6 into main Oct 12, 2023
11 of 12 checks passed
@hadley hadley deleted the req_perform-names branch October 12, 2023 19:34
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.

Rename paginate_req_perform() to req_perform_paginate() etc
2 participants