Add a Page::all method

[hwittenborn]

Would it be possible to add a function such as Page::all that just does something like Octocrab::all_pages(page)? Currently I have to make an intermediate variable when doing something like this to fetch all issues:

let issues - crab.issues()
    ...
    .await?;
let issues = crab.all_pages(issues).await?;

Having a Page::all method would make the code feel a lot cleaner imo, as you don't have that intermediate variable:

let issues = crab.issues()
    ...
    .await?
    .all()
    .await?;

[XAMPPRocky]

Thank you for your issue! While I can appreciate the ergonomics of that, it would require that all pages keep a reference to the HTTP client which would bring a cost, and I don't want to add a hidden cost for just one method. The best we could do is maybe the following, but I'd have to check if the borrow checker allows it.

impl Page<T> {
    async fn all(self, crab: &Octocrab) -> Vec<T> {
        crab.all_pages(self).await
    }
}

let issues = crab.issues()
    ...
    .await?
    .all(&crab)
    .await?;

[hwittenborn]

it would require that all pages keep a reference to the HTTP client which would bring a cost, and I don't want to add a hidden cost for just one method.

You could just do something like this, right?

impl Page<T> {
    async fn all(self, crab: &Octocrab) -> Vec<T> {
        crate::instance().all_pages(self).await
    }
}

That would require the user to set the global instance, but I think adding a note about it using the global instance would be sufficient. I know there's a bit of flexibility lost, but considering there wouldn't be any parameter stuff to deal with then I think it'd be best.

The best we could do is maybe the following, but I'd have to check if the borrow checker allows it.

impl Page<T> {
    async fn all(self, crab: &Octocrab) -> Vec<T> {
        crab.all_pages(self).await
    }
}

The way I'd personally want the API is to not have to think about passing in an Octocrab instance at all - in my mind I consider a Page instance as connected to the active Octocrab instance, and having to pass in an instance manually breaks that mental model for me.

What do you think?

[hwittenborn]

it would require that all pages keep a reference to the HTTP client which would bring a cost, and I don't want to add a hidden cost for just one method.

Are you talking about a maintenance cost or some kind of runtime cost? I wouldn't think the maintenance part would be too much of an issue since you're just adding an extra field to the struct, right?

[XAMPPRocky]

That would require the user to set the global instance

I don't think that's intuitive that this one method requires the global instance, that's not worth it for saving one parameter.

Are you talking about a maintenance cost or some kind of runtime cost?

Runtime and borrowing cost, adding an extra field makes the page bigger, and it will affect the lifetime of the Page as now the Page has to ensure it lives as long as the client.

[hwittenborn]

Runtime and borrowing cost, adding an extra field makes the page bigger, and it will affect the lifetime of the Page as now the Page has to ensure it lives as long as the client.

I suppose it'd be bigger, but I'd think quite negligible since it's just storing a reference to another object. Considering everything else that gets created in a normal program's lifespan I'd think that the space wouldn't make much of a difference in anything.

I don't think the lifetime stuff should be much of an issue, as all the *Handler structs contain references to the Octocrab instance anyway. Considering Page objects usually come out of those *Handler structs I'd think it'd be a suitable solution.

[XAMPPRocky]

I don't think the lifetime stuff should be much of an issue, as all the *Handler structs contain references to the Octocrab instance anyway

Handler's are designed to be short lived, they're only supposed to exist for the length it takes you to set the arguments and then send the request. The data that's returned is expected to live much longer, and may be sent across threads where adding a reference would make that not possible.

[NickLarsenNZ]

IMO, the library should handle returning the complete results. One page is not super useful, especially when there are no helper methods to continue through the paged results.

I tried implementing a stream over paged results, but couldn't get past the methods that consume self...

pub trait GetAllPages<T> {
    async fn get_all_pages(self) -> octocrab::Result<Vec<T>>;
}

impl<'octo, 'r> GetAllPages<Label> for ListLabelsForRepoBuilder<'octo, 'r> {
    async fn get_all_pages(self) -> octocrab::Result<Vec<Label>> {
        use futures::stream::{self, TryStreamExt};

        let stream = stream::try_unfold(1u32, |page_number| async move {
            // this doesn't work because page() and send() take ownership of self
            // but self is needed by the subsequent calls to this FnMut closure
            let result = self.page(page_number).send().await?;

            if result.next.is_some() {
                let next_page_number = page_number + 1;
                let yielded = result.items;
                Ok(Some((yielded, next_page_number)))
            } else {
                Ok(None)
            }
        });

        // might be able to try_flatten and try_collect without the match
        let labels: octocrab::Result<Vec<Vec<Label>>> = stream.try_collect().await;
        let labels = match labels {
            Ok(labels) => Ok(labels.into_iter().flatten().collect()),
            Err(e) => return Err(e),
        };

        labels
    }
}

[NickLarsenNZ]

I take back my previous comment. I have just revisited this and noticed the stream feature which solved my problem.

https://docs.rs/octocrab/0.39.0/octocrab/struct.Page.html#method.into_stream