First impressions

[martincostello]

Hey folks,

I just took a quick look at pulling this into an existing application of mine and I thought I'd just give you some initial feedback. I didn't get very far as it was hundreds of compiler warnings, so just tweaked a small part of it to get a feel for things.

I realise it's very early and some of it might be things you already know about, so no worries if I'm not telling you anything new.

Also this is intended to be neutral feedback on first impressions, don't take it as positive or negative, just "huh, that's different" observations.

• It doesn't currently build - there's bits to do with Actions, Packages and Codespaces that don't compile. For now I just deleted those parts as I didn't need to use them in my app anyway.
• A lot of the member names aren't very ".NET"-y with things like ALLCAPS or have underscores in them. Maybe for a native .NET experience they match the API too closely and look out-of-place for the typical .NET library API surface and need some pre-processing being being emitted as code?
• There's a lot of namespaces and the sub-namespaces often repeat the parent (e.g. Foo.Item.Item.Bar)
• There's lots of dictionaries/indexers being used to index/access into things. Might this have performance implications at scale if lots of dictionaries are being allocated to index through the API to be able to do an HTTP call? This also at first glance makes it look like you might have to check the thing you're indexing exists in advance like with a normal Dictionary<K, V> type, rather than it just being deferred execution collecting values until you get to making the actual request.
• The indexer parameters are all called position. Intellisense when using the API would be easier to reason about if they had the appropriate names (owner, repo, number) etc.
• Some enums have been dropped (or at least I couldn't find them) that might make it hard-er to migrate an existing code base over. The one I hit was RepositoryVisbility which in Octokit encapsulates the possible public, internal and private values.

Here's an example of some of the above in a single snippet of code using the generated API instead of Octokit.net:

using GitHub.Octokit;

// Really really long and repetitive namespace
using GitHub.Octokit.Repos.Item.Item.Pulls.Item.Reviews;

OctokitClient client = CreateClient(...);

var review = new ReviewsPostRequestBody()
{
    Event = ReviewsPostRequestBody_event.APPROVE, // Underscore and all caps type and member name
};

// Lots of indexers
await client.Repos[owner][name].Pulls[number].Reviews.PostAsync(review);

The Octokit equivalent of the above was:

using Octokit;

IGitHubClient client = CreateClient(...);

var review = new PullRequestReviewCreate()
{
    Event = PullRequestReviewEvent.Approve
};

await client.PullRequest.Review.Create(owner, name, number, review);

I hope the observations are useful.

[nickfloyd]

Hey @martincostello thank you so much for the feedback and perspective here! We were hoping for information like this. @kfcampbell and I are going to have a look at what you posted and chat about it to see what we can apply and change. Please keep this perspective coming if you have the time. The whole octokit community will be loads better because of it! ❤

[nickfloyd]

Hey @martincostello,

We've been looking into your thoughts and comments. Thanks again. Just a quick update.

not building. We had some ignore patterns that were colliding with our generated endpoints and models - those should be fixed now.

Naming This is something we'll be tweaking - we went with some plausible defaults for generation - we're definitley going to drive to using PascalCase for publics, camel for private members, and so on. We'll also be implementing linting as a CI gate to prevent deviation.

Namespacing Too right. That has to do with the way things nest, object hierarchy, and the tree walking that has to be done to generate things. Hopefully we can simplify this as we discover more... super good call out though.

dictionaries/indexers We're still looking into this.

indexer parameters are all called position 100%, hopefully we can come up with some formatters that will get this more "right" - wed definitely like to favor expressiveness - especially when it comes to the basics like this!

RepositoryVisbility Missing enums. Yikes, we're still hunting this one down as well. Thanks for the head's up.

[martincostello]

Thanks for taking a look.

Let me know when you've either pushed something to NuGet or pushed a bunch of changes worth re-review and I can take another look at it in one of the repos I use Octokit in.

[martincostello]

Hey folks - I've taken another look at things using the Octokit.NET.SDK NuGet package, and had another go at migrating one of my apps over to use it. Here's some additional observations from the partial changes I made in this commit.

• The convenience property GitHubApiUrl that points to https://api.github.com has gone. This also raises the question to me of whether the SDK correctly supports GHES yet (i.e. the /api/v3/ path prefix). In the current SDK that's pretty seamless if you need to change the base URL as you just change the host and that's pretty much it.
• HttpClientRequestAdapter implements IDisposable but GitHubClient doesn't - should it? Just a thought when refactoring my IoC setup and ensuring things are disposed of appropriately when created through DI where there's different lifetimes (transient, scoped etc.).
• The SDK no longer has a GetLastApiInfo() method you can use to get the last rate limit information from the pull request. What's the recommended path forward going to be for observing those values?
• GitHubClient has a Repos and a Repositories property. While that maps to the GitHub API, it's potentially confusing as to which one should be being used for a given use case.
• I don't seem to be able to find methods for various APIs, such as getting the current authenticated user, commit statuses etc, branch protection settings.
• Are there successors to custom exception types such as ApiException and PullRequestNotMergeableException?
• Of the view "get list" methods I've tried to use, they return Task<IList<T>?> - is it correct that they return null instead of just an empty list, or does that signify "not found"?
• Some of the nullability of the models seems wrong - I wouldn't expect an issue to ever have a null Number for instance. The models are adding a lot more null checking to make the compiler happy at the moment which doesn't seem right in all cases.

  dotnet-sdk/src/GitHub/Models/Issue.cs

  Line 141 in 427198c

  public int? Number { get; set; }