-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add active committers API implementation #2208
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&rdquo 8000 ;, 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 active committers API implementation #2208
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2208 +/- ##
=======================================
Coverage 97.79% 97.80%
=======================================
Files 112 112
Lines 10036 10074 +38
=======================================
+ Hits 9815 9853 +38
Misses 154 154
Partials 67 67
Continue to review full report at Codecov.
|
0b4f992
to
2512b27
Compare
@gmlewis I guess I addressed all the review comments. Please let me know if everything looks alright. Thanks! |
github/billing.go
Outdated
|
||
// ActiveCommitters represents the total active committers across all repositories in an Organization. | ||
type ActiveCommitters struct { | ||
TotalAdvancedSecurityCommitters int `json:"total_advanced_security_committers,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.
If it isn't a reference type (e.g. *int
in this case) then it doesn't need the omitempty
.
TotalAdvancedSecurityCommitters int `json:"total_advanced_security_committers,omitempty"` | |
TotalAdvancedSecurityCommitters int `json:"total_advanced_security_committers"` |
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.
@gmlewis can I know how we find out if its a reference type or not?
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.
@gmlewis can I know how we find out if its a reference type or not?
There are a few scenarios where reference types are needed and/or desired:
- When the response might not populate the field,
- When an option being sent from the client to the server is optional and we don't want the
zero
value of the field to be sent - When we have a slice of a struct, it is desirable to iterate over a slice of pointers rather than a slice of values.
Conversely, if a field is already a reference type (for example, a slice or an interface), we don't typically want to have a pointer to it. In other words, if you ever see *[]something
or *[]*something` then that is typically suspect and not desirable.
I hope that helps.
@gmlewis I guess I addressed all the comments. Please let me know if the changes look alright! Thanks for the review! |
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, @ganeshkumarsv !
LGTM.
Awaiting second LGTM before merging.
(Please note that ALL other contributors to this repo are welcome to provide the second PR review/comment/approval that we need for merging and that we are not waiting for any particular reviewer unless otherwise noted.)
@cpanato @gunadhya @sagar23sj |
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.
looks great to me, thanks for adding that
Thank you, @cpanato ! |
@gmlewis can we have a minor release? |
@ganeshkumarsv - this change is now incorporated in the new release: |
New Method for fetching active committers for an Organization
https://docs.github.com/en/rest/reference/billing#get-github-advanced-security-active-committers-for-an-organization