-
Notifications
You must be signed in to change notification settings - Fork 2.1k
refactor!: Change required params from pointers to values #3654
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
base: master
Are you sure you want to change the base?
refactor!: Change required params from pointers to values #3654
Conversation
Please follow step 4 in CONTRIBUTING.md and push the changes to this PR. |
update exmaples
d124f0e
to
826440e
Compare
done ✔️ @gmlewis |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3654 +/- ##
=========================================
Coverage ? 91.24%
=========================================
Files ? 184
Lines ? 16299
Branches ? 0
=========================================
Hits ? 14872
Misses ? 1245
Partials ? 182 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
if ref == nil { | ||
return nil, nil, errors.New("reference must be provided") | ||
} | ||
func (s *GitService) CreateRef(ctx context.Context, owner string, repo string, ref Reference) (*Reference, *Response, error) { |
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.
Since we are breaking compatibility here, I propose refactoring:
// CreateRef represents the payload for creating a reference.
type CreateRef struct {
Ref string `json:"ref"`
SHA string `json:"sha"`
}
func (s *GitService) CreateRef(ctx context.Context, owner string, repo string, ref CreateRef) (*Reference, *Response, error) {
u := fmt.Sprintf("repos/%v/%v/git/refs", owner, repo)
req, err := s.client.NewRequest("POST", u, ref)
if err != nil {
return nil, nil, err
}
// ...
What do you think?
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.
Sounds good to me. And while you are here, this should be owner, repo string
instead of owner string, repo string
.
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.
And while you are here, this should be
owner, repo string
instead ofowner string, repo string
.
Fixed in a separate PR #3655
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.
can this change come in a separate PR or should I ship this in this PR? I see that this would require some heavy changes in test cases and I am thinking it might be out of scope of this PR? what do you guys think?
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.
Since this is already a disruptive API-breaking PR, I suggest we go ahead and address it here within this PR instead of breaking the API twice.
BREAKING CHANGE:
GitService
endpoints now pass required params by-value instead of by-ref.What problem are you solving?
Notes
This is most likely going to be a multi PR issue, since this is my first time working on this codebase. I started by picking a "service" area ( Git ) and converted each function that I thought met the criteria set in the issue mentioned, which are: