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

Initial implementation of get_all() for async using Stream #200

Merged
merged 14 commits into from
Oct 28, 2021

Conversation

phayes
Copy link
Contributor

@phayes phayes commented Oct 22, 2021

This is an initial implementation of List::get_all() for async using Stream.

It needs testing, but I think the general approach is correct. I'll make a note when I feel like I've tested it enough for merging.

Once tested and merged, this will resolve #41

@phayes phayes changed the title Initial implementation of get_all for async using Stream Initial implementation of get_all() for async using Stream Oct 23, 2021
@phayes phayes changed the title Initial implementation of get_all() for async using Stream Initial implementation of get_all() for async using Stream Oct 23, 2021
@phayes phayes marked this pull request as draft October 23, 2021 16:47
@arlyon
Copy link
Collaborator

arlyon commented Oct 25, 2021

This is great, thanks for the work here. Only suggestion is to reduce nesting in the unfold:

pub fn get_all(self, client: &Client) -> impl TryStream<Ok = T, Error = Error> {
    // We are going to be popping items off the end of the list, so we need to reverse it.
    let mut init_list = self;
    init_list.data.reverse();

    futures_util::stream::unfold(Some((init_list, client.clone())), |state| async move {
        let (mut list, client) = state?; // if none, we sent the last item in the list last iteration
        let val = list.data.pop()?; // the initial list was empty, so we're done.

        if !list.data.is_empty() {
            return Some((Ok(val), Some((list, client)))); // some value on this page that isn't the last value on the page
        }

        if !list.has_more {
            return Some((Ok(val), None)); // final value of the stream, no errors
        }

        match list.next(&client).await {
            Ok(mut next_list) => {
                next_list.data.reverse();

                // Yield last value of this page, the next page (and client) becomes the state
                Some((Ok(val), Some((next_list, client))))
            }
            Err(e) => Some((Err(e), None)), // we ran into an error. the last value of the stream will be the error.
        }
    })
}

Otherwise, thanks for the contribution! Are you going to keep both PRs up or close this one in favour of #194?

@phayes phayes marked this pull request as ready for review October 27, 2021 16:44
@phayes
Copy link
Contributor Author

phayes commented Oct 27, 2021

Thanks for the improvements @arlyon .

I've now tested this enough to satisfy myself that it's correct and is ready for merging.

@arlyon , as to your question, I created a separate PR since this PR is a lot smaller than #194. The other PR also contains these changes, but also includes far more invasive changes that might require more careful review. So it might make sense to review and merge this PR first if you're happy with it, then merge #194 after you've had the time to review the more complex changes there.

Or you could just merge #194 and be done with it all. :)

@arlyon
Copy link
Collaborator

arlyon commented Oct 28, 2021

Agreed, I'll merge and then you can rebase. Thanks for the contribution!

@arlyon arlyon merged commit 3a80928 into wyyerd:master Oct 28, 2021
@phayes
Copy link
Contributor Author

phayes commented Oct 28, 2021

Thanks @arlyon ! #194 has been rebased and is ready as well.

dav1do added a commit to DreamStageLive/stripe-rs that referenced this pull request Nov 23, 2021
- Combine our detach with version added in wyyerd#141
- Fix get_next params from wyyerd#200 (confused about this still)
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.

Implement List::get_all() -> Stream<T> for feature = "async"
2 participants