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 active committers API implementation #2208

Merged

Conversation

ganeshkumarsv
Copy link
Contributor

@ganeshkumarsv ganeshkumarsv commented Nov 23, 2021

@google-cla google-cla bot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Nov 23, 2021
@ganeshkumarsv ganeshkumarsv marked this pull request as draft November 23, 2021 21:40
@codecov
Copy link

codecov bot commented Nov 23, 2021

Codecov Report

Merging #2208 (54c9873) into master (b26fa8f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           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           
Impacted Files Coverage Δ
github/billing.go 100.00% <100.00%> (ø)
github/actions_workflow_runs.go 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b26fa8f...54c9873. Read the comment docs.

@ganeshkumarsv ganeshkumarsv force-pushed the ganesh.kumar-billing-codescanning branch from 0b4f992 to 2512b27 Compare November 23, 2021 23:00
@ganeshkumarsv ganeshkumarsv marked this pull request as ready for review November 23, 2021 23:14
github/billing.go Outdated Show resolved Hide resolved
github/billing.go Outdated Show resolved Hide resolved
github/billing_test.go Show resolved Hide resolved
github/billing.go Outdated Show resolved Hide resolved
github/billing.go Outdated Show resolved Hide resolved
github/billing.go Outdated Show resolved Hide resolved
github/billing.go Outdated Show resolved Hide resolved
@ganeshkumarsv
Copy link
Contributor Author

@gmlewis I guess I addressed all the review comments. Please let me know if everything looks alright. Thanks!

github/billing.go Outdated Show resolved Hide resolved
github/billing.go Outdated Show resolved Hide resolved
github/billing.go Outdated Show resolved Hide resolved

// ActiveCommitters represents the total active committers across all repositories in an Organization.
type ActiveCommitters struct {
TotalAdvancedSecurityCommitters int `json:"total_advanced_security_committers,omitempty"`
Copy link
Collaborator

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.

Suggested change
TotalAdvancedSecurityCommitters int `json:"total_advanced_security_committers,omitempty"`
TotalAdvancedSecurityCommitters int `json:"total_advanced_security_committers"`

Copy link
Contributor Author

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?

Copy link
Collaborator

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.

github/billing.go Outdated Show resolved Hide resolved
github/billing.go Outdated Show resolved Hide resolved
github/billing.go Outdated Show resolved Hide resolved
github/billing.go Outdated Show resolved Hide resolved
github/billing.go Outdated Show resolved Hide resolved
github/billing.go Outdated Show resolved Hide resolved
@ganeshkumarsv
Copy link
Contributor Author

@gmlewis I guess I addressed all the comments. Please let me know if the changes look alright! Thanks for the review!

@gmlewis gmlewis changed the title add active committers api implementation Add active committers API implementation Nov 27, 2021
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, @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.)

@ganeshkumarsv
Copy link
Contributor Author

@cpanato @gunadhya @sagar23sj
I see you have contributed to billing.go and billing_test.go. Can anyone please review and approve my PR? 🙇

Copy link
Contributor

@cpanato cpanato left a 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

@gmlewis
Copy link
Collaborator

gmlewis commented Nov 29, 2021

Thank you, @cpanato !
Merging.

@gmlewis gmlewis merged commit f0f6761 into google:master Nov 29, 2021
@ganeshkumarsv
Copy link
Contributor Author

@gmlewis can we have a minor release?

@gmlewis
Copy link
Collaborator

gmlewis commented Nov 29, 2021

@gmlewis can we have a minor release?

I will work on a release, but due to the breaking API change in #2205, it will be a major release.

@gmlewis
Copy link
Collaborator

gmlewis commented Nov 29, 2021

@ganeshkumarsv - this change is now incorporated in the new release:
https://github.com/google/go-github/releases/tag/v41.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants