Skip to content

Adding Packages Api#2551

Merged
nickfloyd merged 7 commits intooctokit:mainfrom
JonruAlveus:2360_PackageApi
Sep 8, 2022
Merged

Adding Packages Api#2551
nickfloyd merged 7 commits intooctokit:mainfrom
JonruAlveus:2360_PackageApi

Conversation

@JonruAlveus
Copy link
Contributor

fixes #2360

Copy link
Contributor

@timrogers timrogers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to add this ❤️ I've reviewed commit-by-commit and it looks good to me from an API and basic implementation perspective, but I'm not a .NET expert, so let's wait for @nickfloyd to return from holiday before we merge (if that's okay!).

Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this all looks fantastic @JonruAlveus - other than the typos, this looks g2g! ❤️

{
public interface IObservablePackagesClient
{
IObservablePackageVersionsClient PackageVersions { get; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is one of those weird API domains where package versions are a subset of packages, yet a package version is a package.

What you've done here feels like it works - I'm glad you approached the relationship this way to help reduce namespace confusion - I think leaving PackageVersions out of this interface would've made things a bit more clumsy.

github.Packages.PackageVersions.GetAllForUser
github.Packages.GetAllForOrg

feels more elegant than something like (though possible):

github.PackageVersions.GetAllForUser
github.Packages.GetAllForOrg

While, as you've implemented, it can lead to a bit more verbose API call, I think the expression feels more natural ❤️ solid work here!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! It is a little odd. I took my inspiration from the GitHub Api docs where it's all grouped together in one Packages section.

I've just pushed a final fix for the typos also.

@nickfloyd nickfloyd merged commit cf9db5f into octokit:main Sep 8, 2022
@nickfloyd
Copy link
Contributor

release_notes: Added Packages and Package versions APIs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Packages API

3 participants