Add AsyncPaginationExtensions

[Saalvage]

Compared to my original design draft I've decided to opt for a slightly more complex approach involving a class dedicated to providing both sequential as well as random access. It caches everything to make multiple enumerations (that possibly all don't iterate to the very end, saving resources) faster. Ultimately the cost to this should be minimal, as cacheless sequential access likely allocates the same amount of memory, just freeing it immediately afterwards.

I've written a script to generate the Extensions.cs file as it is basically just a bunch of boilerplate.
Here it is in its entirety:

using System.Text.RegularExpressions;

var root = args.ElementAtOrDefault(0);
while (root == null) {
    Console.WriteLine("Please enter the octokit project folder:");
    root = Console.ReadLine();
}

if (args.Length == 0) {
    Console.Clear();
}

var enumOptions = new EnumerationOptions { RecurseSubdirectories = true };
var argSplitRegex = new Regex(@" (?![^<]*>)");
var paginatedCallRegex = new Regex(@".*Task<IReadOnlyList<(?<returnType>\w+)>>\s*(?<name>\w+)(?<template><.*>)?\((?<arg>.*?)(, )?ApiOptions \w*\);");

Console.Write(
@"using System;
using System.Collections.Generic;

namespace Octokit.AsyncPaginationExtension
{
    /// <summary>
    /// Provides all extensions for pagination.
    /// </summary>
    /// <remarks>
    /// The <code>pageSize</code> parameter at the end of all methods allows for specifying the amount of elements to be fetched per page.
    /// Only useful to optimize the amount of API calls made.
    /// </remarks>
    public static class Extensions
    {
        private const int DEFAULT_PAGE_SIZE = 30;
");

var first = true;
foreach (var file in Directory.EnumerateFiles(root, "I*.cs", enumOptions)) {
    var type = Path.GetFileNameWithoutExtension(file);

    var firstDef = true;
    foreach (var line in File.ReadAllLines(file)) {
        var match = paginatedCallRegex.Match(line);
        if (!match.Success) { continue; }

        if (firstDef) {
            firstDef = false;
            if (first) {
                first = false;
            } else {
                Console.WriteLine();
            }
        }

        var returnType = match.Groups["returnType"].Value;
        var name = match.Groups["name"].Value;
        var arg = match.Groups["arg"].Value;
        var template = match.Groups["template"];
        var templateStr = template.Success ? template.Value : string.Empty;

        var splitArgs = argSplitRegex.Split(arg).ToArray();

        var lambda = arg.Length == 0
            ? $"t.{name}{templateStr}"
            : $"options => t.{name}{templateStr}({string.Join(' ', splitArgs.Where((_, i) => i % 2 == 1))}, options)";

        var docArgs = string.Join(", ", splitArgs.Where((_, i) => i % 2 == 0)).Replace('<', '{').Replace('>', '}');
        if (docArgs.Length != 0) {
            docArgs += ", ";
        }
        
        if (arg.Length != 0) {
            arg += ", ";
        }

        Console.Write($@"
        /// <inheritdoc cref=""{type}.{name}({docArgs}ApiOptions)""/>
        public static IPaginatedList<{returnType}> {name}Async{templateStr}(this {type} t, {arg}int pageSize = DEFAULT_PAGE_SIZE)
            => pageSize > 0 ? new PaginatedList<{returnType}>({lambda}, pageSize) : throw new ArgumentOutOfRangeException(nameof(pageSize), pageSize, ""The page size must be positive."");
");
    }
}

Console.Write(
@"    }
}
");

I'm not sure whether it might be preferable to use CI to generate the Extensions.cs file to prevent it from becoming out of sync with the main project. And if not that whether this generator code should at least find a more permanent place somewhere in the repo (if so, where?)

A few additional points of concern:

• I've chosen the lowest possible .NET versions to support what I need, which results in the Microsoft.Bcl.AsyncInterfaces package being required. Especially since octokit itself is moving onto a more modern version this choice should potentially be revisited.
• I've opted for using <LangVersion>9</LangVersion> as this allows for some cleaner as well as less restrictive code in a lot of places. I'm not sure whether this is actually relevant to library consumers at all.
• I'm honestly not entirely sure about the contents of the .csproj file, I've based it on the existing ones, but for some reason the Reference Include tags were causing errors which is why I removed them. I'd appreciate the .csproj file being taken a closer look at.
• I've decided to place all extension methods in a single class, allowing for easier importing. If there is some downside to this that I'm not aware of the generation code could easily be adjusted to create various files mirroring the main project's file structure.
• I'm not entirely sure if the tests I've written are enough, but since the extension methods are just wrappers all following the same pattern testing all of them seems excessive.

If there's anything I can do to help with this being integrated please let me know! :)

Closes #2463

[nickfloyd]

Thank you for putting in the effort and time on this - it's looking great! ❤️

Regarding your questions and concerns:

I'm not sure whether it might be preferable to use CI to generate the Extensions.cs file to prevent it from becoming out of sync with the main project. And if not that whether this generator code should at least find a more permanent place somewhere in the repo (if so, where?)

Given that we are headed down to a more generative SDK I feel like... (see the discussions here and here and here for more info on that)

IMO the preferred approach would be to have a generators project with things like your script above namespaced i.e.
Octokit.Generators and the generator above would be something like Octokit.Generators.AsyncPaginationExtension or something.

This way we're establishing the future location for all generators now to remove this question later. Also, this will allow us to use CI to build the generators, generate the code, and commit it before the other aspects of the SDK are built.

As a side note, in my perfect world brain, all models and related libs would be hoisted out of this SDK and referenced in as a package so that we could better support versioning and be able to ship SDK core changes separate from SDK model changes.

For now, let's create a generators project, add the script above, then document how to run and what to expect - we can do a follow-up for CI to implement this in a more automated way.

Let me know what your thoughts.

I've chosen the lowest possible .NET versions to support what I need, which results in the Microsoft.Bcl.AsyncInterfaces package being required. Especially since octokit itself is moving onto a more modern version this choice should potentially be revisited.

This seems reasonable - in the future, we'll be looking at using Roslyn's source generation and templates as a more future-proof way to generate code from things like the OpenAPI descriptions, etc... the work you've done here just gets us all closer to that future! 💥

I've opted for using 9 as this allows for some cleaner as well as less restrictive code in a lot of places. I'm not sure whether this is actually relevant to library consumers at all.

✅

I'm honestly not entirely sure about the contents of the .csproj file, I've based it on the existing ones, but for some reason the Reference Include tags were causing errors which is why I removed them. I'd appreciate the .csproj file being taken a closer look at.

The .csproj file looks fine to me... we haven't made the jump to .NET Standard 2.0 just yet - I'm working on that this week but I don't think that would cause the problems that you've described - do you have the error output from the compiler? If so post it and I'll have a look.

I've decided to place all extension methods in a single class, allowing for easier importing. If there is some downside to this that I'm not aware of the generation code could easily be adjusted to create various files mirroring the main project's file structure.

❤️ Yeah I love this. It isolates the paging concerns and it keeps things tidy (with generation and with other classes) 👍

I'm not entirely sure if the tests I've written are enough, but since the extension methods are just wrappers all following the same pattern testing all of them seems excessive.

Yeah, I agree it's nuanced and shouldn't be overblown. I feel like the tests you have are adequate. We might want tests around the generator itself just to assert it is doing what you expect it to do.

[nickfloyd]

I left some comments about the generator being in an actual project and adding some docs on usage. Let me know your thoughts on that. Thanks again for all of the excellent work here! ❤️

Also, I'd be glad to make those changes if you just want to get the ones you've already done in this PR merged in. Let me know your thoughts!

[Saalvage]

Hey, thanks for the extensive review! I'm currently in the middle of my exams, so I likely won't be able to get back to this for a few weeks, sorry for the inconvenience! I would definitely be up to implement the changes in question once I have the time to do so, but if you're able to scrape the time together to do them yourself in that time I would not object! :)

[nickfloyd]

@Saalvage sounds good... I'll move to merge this in and then do a fast follow on the generator project. Thank you for all of the work you've put in here... I wish you all the best on your exams too! I'm sure you'll do amazing! 🏫 🎓

[nickfloyd]

As a follow up: #2534

[nickfloyd]

Looks good, thanks for getting the ball rolling here! I'll do a follow-up that will implement the generators and eventually some CI / build automation.

[nickfloyd]

release_notes: Added an asynchronous paging extension that adds paging for all list-based response objects