Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add WebhookTypes and EventForType methods #2865

Merged
merged 9 commits into from Aug 16, 2023
Merged

Conversation

evankanderson
Copy link
Contributor

Fixes #2863

github/event.go Outdated Show resolved Hide resolved
github/messages.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Could you please also add tests for requesting a nil event, an unknown event, and any other place where we pass in garbage to make sure it behaves properly in all cases?

@evankanderson
Copy link
Contributor Author

I switched to a bit of judicious reflection for the struct copy and pre-computation for the rest of the mappings, and now this is a nicely negative total lines of code PR. 😁

github/event.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #2865 (848850a) into master (0c10d67) will decrease coverage by 0.02%.
Report is 4 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2865      +/-   ##
==========================================
- Coverage   98.07%   98.05%   -0.02%     
==========================================
  Files         139      139              
  Lines       12357    12253     -104     
==========================================
- Hits        12119    12015     -104     
  Misses        162      162              
  Partials       76       76              
Files Changed Coverage Δ
github/event.go 100.00% <100.00%> (ø)
github/messages.go 100.00% <100.00%> (ø)
github/repos_hooks_deliveries.go 92.45% <100.00%> (ø)

... and 2 files with indirect coverage changes

github/messages.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @evankanderson !
A few more tweaks, please, then I think we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.

github/event.go Outdated Show resolved Hide resolved
github/messages.go Outdated Show resolved Hide resolved
github/messages.go Show resolved Hide resolved
github/messages.go Outdated Show resolved Hide resolved
@evankanderson
Copy link
Contributor Author

Ready for another look / another reviewer, I think.

github/event.go Outdated Show resolved Hide resolved
@evankanderson
Copy link
Contributor Author

Do you want me to flatten my commits?

@gmlewis
Copy link
Collaborator

gmlewis commented Aug 15, 2023

Do you want me to flatten my commits?

No need to flatten any commits, as we always "squash and merge" in this repo. Thank you, though.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

@evankanderson - this looks great to me!
Thank you for your patience and your willingness to experiment with this one.
I think this looks super clean and will help with reducing not only the maintenance burden of this repo, but also make it much easier for contributors to add new event types as GitHub keeps cranking them out. 😁

LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Aug 15, 2023
@gmlewis
Copy link
Collaborator

gmlewis commented Aug 15, 2023

@googlebot - why have you been silent on this PR? What happened to your automated CLA checks?!?

@gmlewis
Copy link
Collaborator

gmlewis commented Aug 15, 2023

Ah, is @googlebot dead and been replaced by @google-cla instead?

@gmlewis
Copy link
Collaborator

gmlewis commented Aug 15, 2023

@google-ospo-team - can you please investigate why this PR was never checked by the @google-cla bot?
I can't move forward on this PR until it verifies the CLA is signed by all parties.
Thank you!

@gmlewis
Copy link
Collaborator

gmlewis commented Aug 15, 2023

CLA has been cleared. Thank you, @google-ospo-team !

Now awaiting second LGTM+Approval from any other contributor to this repo before merging.

@gmlewis
Copy link
Collaborator

gmlewis commented Aug 16, 2023

@Parker77 - do you have time for a code review, please?

github/messages.go Outdated Show resolved Hide resolved
Co-authored-by: Parker77 <20973702+Parker77@users.noreply.github.com>
Copy link

@Parker77 Parker77 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Aug 16, 2023
@gmlewis
Copy link
Collaborator

gmlewis commented Aug 16, 2023

Thank you, @Parker77 !
Merging.

@gmlewis gmlewis merged commit 1a4b106 into google:master Aug 16, 2023
8 of 9 checks passed
gmlewis pushed a commit to gmlewis/go-github that referenced this pull request Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support enumerating the known event types
3 participants