-
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
Change type of HookDelivery.InstallationID to Int64. #2235
Change type of HookDelivery.InstallationID to Int64. #2235
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2235 +/- ##
=======================================
Coverage 97.79% 97.79%
=======================================
Files 113 113
Lines 10156 10156
=======================================
Hits 9932 9932
Misses 156 156
Partials 68 68
Continue to review full report at Codecov.
|
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.
I'm wondering why these fields are pointers but don't have omitempty
?
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.
I've looked through the code and can't find an explanation for why the fields in the HookDelivery
do not have an omitempty
even though they are all references.
So while we are breaking the API, please go ahead and add omitempty
to all the fields in lines 20-30. Thank you!
@gmlewis yeah it looks like it was added on the |
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, @pierce-m !
LGTM.
Awaiting second LGTM from any other repo contributor before merging.
Is there anyone else who can provide review? Doesn't seem like @wesleimp is very active any more. |
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 ! |
Resolves #2234