[WIP] implement first-class paging support

[shiftkey]

🌵 🌵 🌵 🌵 🌵 🌵 🌵 🌵 🌵 WORK IN PROGRESS 🌵 🌵 🌵 🌵 🌵 🌵 🌵 🌵 🌵
🌵 🌵 🌵 🌵 🌵 🌵 🌵 🌵 🌵 DO NOT MERGE 🌵 🌵 🌵🌵 🌵 🌵 🌵 🌵 🌵

I'd been noodling around with how to tackle paging in Octokit, and here's my first attempt at a solution.

The Problem

By default, if there's new data to fetch, the Octokit client will go and fetch it. Thanks to the pagination APIs, this is rather easy to do. For my use case, that's been Good Enough. But I'm at the point where I'd like to do things like:

• adjust the number of results per page
• stop at a certain page number
• skip certain pages
• opt-in to preview API behaviour by overriding accepts header

Some endpoints support other sorts of filtering (for example, issues search has a since parameter. I'm not worrying about those here - this approach should work for all endpoints that return arrays of results.

This isn't something I've focused on given I spent most of my days working with desktop apps, but I know other platforms would love having this level of control to conserve data.

And we have #259 and #625 as open issues, so why not do try and resolve this?

The Approach

You can await any type in C# with the presence of async/await - and it's pretty easy once you know how to get started. For backwards-compatibility, I didn't want to force you to update your code - if you didn't opt-in to this behaviour, you should get the existing behaviour.

So I started with this ideal API:

var repositories = await client.Repository.GetAllForCurrent()
    .WithPageCount(10)
    .SelectPages(10);

Instead of returning Task<IReadOnlyList<T>> here I'm returning ILazyRequest<T> - and it's this type that has the extra options necessary. These is how one can tweak the internal state of the request before it executes.

To get the await magic to work, you need to define a public static method in a static type that takes your type as an extension method and returns a TaskAwaiter<T>.

public static class CustomAwaiter
{
    public static TaskAwaiter<IReadOnlyList<T>> GetAwaiter<T>(this ILazyRequest<T> request)
    {
        Ensure.ArgumentNotNull(request, "request");

        return request.ToTask();
    }
}

This lives under the Octokit namespace, so it should Just Work with existing users.

ToTask() just defers to the existing behaviour, but this gives me the hook necessary to inject the additional parameters before executing the request.

There's also some enhancements necessary to IApiPagination to allow clients to opt-out to paging early. I should be able to extract this dependency from ApiConnection by the time this is done, so I'm fine with changing that behaviour around.

This is the API I've settled on:

var repositories = await github.Repository.GetAllForCurrent()
    .WithOptions(new ApiOptions { PageSize = 10, PageCount = 1 });

Limitations

I need to think about how to do this with the Observable-based APIs - currently we just cheat and transform the Task-based API, but because we surface an IObservable<T> we'd need to do something similar here. Plus there are extension methods like Skip and Take available that we should be respecting (and not fetching all the records in the first place). Perhaps this is when the two libraries diverge, implementation-wise 😢

EDIT: we actually use a different implementation inside the GetAlls for the observable clients - GetAndFlattenAllPages - so I'm gonna put this question aside for another day.

TODO

• unbreak all the tests
• resolve muted fx:cop: issues
• ILazyRequest -> IDeferredRequest
• finish RepositoriesClient migration
• experiment with content responses in Issue Comments
• [ ] ponder on how to do this with Octokit.Reactive 👢 punt to another time

[shiftkey]

I'll pause here before going further and see if people have feedback on this approach.

[shiftkey]

This might also be how we support different media types in the response without hard-coding 💩 everywhere. Added a task to explore this further.

cc @ammeep @thedillonb

[shiftkey]

Went back and forth with @JakeGinnivan about this API stuff, and he pointed out that this change will make the task returned from DeferredRequest cold. And that's going against the Task-based Asynchronous Pattern book which recommends starting the task as early as possible.

"All tasks returned from TAP methods must be “hot.” If a TAP method internally uses a Task’s constructor to instantiate the task to be returned, the TAP method must call Start on the Task object prior to returning it. Consumers of a TAP method may safely assume that the returned task is “hot,” and should not attempt to call Start on any Task returned from a TAP method. Calling Start on a “hot” task will result in an InvalidOperationException (this check is handled automatically by the Task class)."

I don't think this is impossible to do within our current infrastructure, but right now I need to be mindful of this.

[shiftkey]

So I could 🔥 this awaiter and make the user call .ToTask() for these methods.

It's less magical, but takes away the hot/cold issue.

[JakeGinnivan]

Still a breaking change. The API itself has changed from hot to cold and existing code now needs to .ToTask()

Right?

[shiftkey]

Yeah, I'm not sure I can magic up a type here that's both Hot and also modifiable using WithOptions.

So I'm weighing up varying degrees of breaking changes to see how they feel.

[shiftkey]

The API itself has changed from hot to cold and existing code now needs to .ToTask()

Yes, any existing API calls will no longer magically work with await. So you're updating anyway.

[shiftkey]

Been thinking on different approaches. Closing this one for now.