dotnet-sdk icon indicating copy to clipboard operation
dotnet-sdk copied to clipboard

First impressions

Open martincostello opened this issue 7 months ago • 4 comments

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.

martincostello avatar Dec 07 '23 15:12 martincostello