-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
There was a problem hiding this 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?
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. 😁 |
Codecov Report
@@ 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
|
There was a problem hiding this 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.
Ready for another look / another reviewer, I think. |
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. |
There was a problem hiding this 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.
@googlebot - why have you been silent on this PR? What happened to your automated CLA checks?!? |
Ah, is @googlebot dead and been replaced by @google-cla instead? |
@google-ospo-team - can you please investigate why this PR was never checked by the @google-cla bot? |
CLA has been cleared. Thank you, @google-ospo-team ! Now awaiting second LGTM+Approval from any other contributor to this repo before merging. |
@Parker77 - do you have time for a code review, please? |
Co-authored-by: Parker77 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thank you, @Parker77 ! |
Fixes #2863