Conversation
mgirlich
reviewed
Oct 27, 2023
Collaborator
|
Looks good to me. We need to think about error handling. It should probably be the same as in |
Member
Author
|
Yeah, I'll have a go at #310. I think an enum is the way forward. I've written up https://design.tidyverse.org/boolean-strategies.html to hopefully help me avoid this problem again in the future. |
#Conflicts: # NEWS.md # R/resp-status.R
hadley
commented
Oct 29, 2023
| #' | ||
| #' @inheritParams req_perform_parallel | ||
| #' @inheritParams req_perform_iteratively | ||
| #' @param on_error What should happen if one of the requests throws an |
Member
Author
There was a problem hiding this comment.
@mgirlich let me know what you think of this error handling, and if it looks good, I'll port to req_perform_parallel().
Member
Author
There was a problem hiding this comment.
And req_perform_iterative(), although that won't have the "continue" option.
#Conflicts: # NEWS.md
mgirlich
reviewed
Oct 30, 2023
mgirlich
reviewed
Oct 30, 2023
mgirlich
reviewed
Oct 30, 2023
mgirlich
reviewed
Oct 30, 2023
mgirlich
reviewed
Oct 30, 2023
mgirlich
approved these changes
Oct 30, 2023
Co-authored-by: Maximilian Girlich <maximilian.girlich@metoda.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #303. Fixes #310
@mgirlich this is just a quick proof of concept. If you think it looks good, I'll finish off the docs and tests. I did try allowing a
NULLreqinreq_perform_iteratively(), but it ended up feeling very artificial since thenext_req()callback would then ignore both its arguments.If we have
req_perform_parallel(),req_perform_sequential(), then I think the matching name would bereq_perform_iterative(), not iteratively.