Handle package webhook #1569
Handle package webhook #1569
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #1569 +/- ##
==========================================
+ Coverage 67.90% 67.95% +0.04%
==========================================
Files 95 96 +1
Lines 8750 8762 +12
==========================================
+ Hits 5942 5954 +12
Misses 1898 1898
Partials 910 910
Continue to review full report at Codecov.
|
github/packages.go
Outdated
| Prerelease *bool `json:"prerelease,omitempty"` | ||
| CreatedAt *Timestamp `json:"created_at,omitempty"` | ||
| UpdatedAt *Timestamp `json:"updated_at,omitempty"` | ||
| PackageFiles *[]PackageFile `json:"package_files,omitempty"` |
There was a problem hiding this comment.
I think the type should be []*PackageFile. Wait for somebody else to confirm it though.
There was a problem hiding this comment.
Yes, deifinitely. Since a slice is already a reference type, we don't typically use pointers to slices.
Please change this to []*PackageFile.
There was a problem hiding this comment.
Thank you, @KrammerJake and @jeet-parekh!
This looks great! Just a couple minor tweaks, please, and then we will be ready for a second LGTM and merging.
github/packages.go
Outdated
| Prerelease *bool `json:"prerelease,omitempty"` | ||
| CreatedAt *Timestamp `json:"created_at,omitempty"` | ||
| UpdatedAt *Timestamp `json:"updated_at,omitempty"` | ||
| PackageFiles *[]PackageFile `json:"package_files,omitempty"` |
There was a problem hiding this comment.
Yes, deifinitely. Since a slice is already a reference type, we don't typically use pointers to slices.
Please change this to []*PackageFile.
github/packages.go
Outdated
| DownloadURL *string `json:"download_url,omitempty"` | ||
| ID *int64 `json:"id,omitempty"` | ||
| Name *string `json:"name,omitempty"` | ||
| Sha256 *string `json:"sha256,omitempty"` |
There was a problem hiding this comment.
Since "SHA" is "Secure Hash Algorithm", let's please make these "SHA256" and "SHA1".
| Repo *Repository `json:"repository,omitempty"` | ||
| Org *Organization `json:"organization,omitempty"` |
There was a problem hiding this comment.
Use full names here? Repository and Organization instead of Repo and Org. For the sake of consistency.
There was a problem hiding this comment.
This file does have mixed naming conventions where both the full name and shorthand are used; however, the shorthand (Repo and Org) versions appear to be the more popular names in this file. I think we can leave these the way they are unless we're trying to standardize to the full name going forward.
There was a problem hiding this comment.
Yes, we have not been totally consistent in this regard, so I didn't comment on these. I think that these are the two names we regularly allow to be shortened, as they are globally well known throughout the package and have very small chance of ever causing conflicts with new JSON tags that might show up in later releases.
I'm fine with leaving them as Repo and Org.
There was a problem hiding this comment.
Thank you, @KrammerJake!
LGTM.
Awaiting second LGTM before merging.
| Repo *Repository `json:"repository,omitempty"` | ||
| Org *Organization `json:"organization,omitempty"` |
There was a problem hiding this comment.
Yes, we have not been totally consistent in this regard, so I didn't comment on these. I think that these are the two names we regularly allow to be shortened, as they are globally well known throughout the package and have very small chance of ever causing conflicts with new JSON tags that might show up in later releases.
I'm fine with leaving them as Repo and Org.
|
Thank you, @wesleimp! |
Addresses #1550