Fix PullRequests.Merge incorrectly sending empty optional values. #501
Conversation
If s.client.NewRequest("PUT", u, pullRequestBody) failed and returned a
nil request and non-nil error, doing req.Header.Set would panic. Need
to check error from NewRequest first thing.
Most importantly, document all optional fields as such.
All 3 options are optional, and should not be sent as part of the JSON payload when they're left as empty strings.
|
CI passes for 1.7 and tip, but fails for 1.6 and older because I used subtests, forgetting they did not exist before 1.7. :( I guess I have no choice but to rid of subtests then, do I? |
|
It LGTM. |
There was a problem hiding this comment.
As for the subtests and supporting Go 1.6, I have a gut feeling that it is too early to force people to change their compiler just for this package, and would recommend that it be rewritten to support the older test mechanism... but I could be convinced either way.
If you think it is OK to force compiler migration to 1.7+, then maybe a separate PR could be introduced that changes the .travis.yml file and explains the rationale for moving to make subtests possible. After that PR gets feedback and merged, we could then merge these testing changes.
Your call.
Thank you for this PR, @shurcooL!
| @@ -301,11 +301,11 @@ type pullRequestMergeRequest struct { | |||
| func (s *PullRequestsService) Merge(owner string, repo string, number int, commitMessage string, options *PullRequestOptions) (*PullRequestMergeResult, *Response, error) { | |||
| u := fmt.Sprintf("repos/%v/%v/pulls/%d/merge", owner, repo, number) | |||
|
|
|||
| pullRequestBody := &pullRequestMergeRequest{CommitMessage: &commitMessage} | |||
| pullRequestBody := pullRequestMergeRequest{CommitMessage: commitMessage} | |||
There was a problem hiding this comment.
I don't agree with making pullRequestBody a value rather than a pointer.
Although this code is typically not performance critical, it is not necessary to make copies of structs (and therefore make copies of its members) on function calls and I think we want to demonstrate the more efficient version.
Additionally, this change would be inconsistent with the rest of this file using pointers, and more generally with the go-github package.
There was a problem hiding this comment.
Hearing this kind of rationale for why we're using pointer is exactly why I did it, thanks @gmlewis. :)
At least I have a reason for why it's there now.
However, I'd be curious to put that claim to the test and benchmark. The struct contains 4 strings, and strings are quite inexpensive to copy (they're a 16-byte struct on 64-bit systems). So if I'm not mistaken, it's a difference of copying a 4*16 = 64 byte struct on the stack, vs allocating an 8 byte pointer on the heap. I don't know off the top of my head how the two would compare in performance and allocations, and I'd be curious to write a bench to learn more about that.
But, definitely not as part of this PR. I'll revert the change for this PR.
|
I agree making 1.7 the min version supported by Travis is too early and not a viable option. I'll remove subtests to make it work everywhere. I'll push 2 fixup commits for you to review, and once you've done that, please give me a chance to rebase them before merging, because I'd like to merge this PR as the 4 logical commits instead of squashing into 1. |
Keeping it a pointer for now for consistency and because I don't yet have evidence it's equally performant (it might not be). Also because this is consistent with the rest of the codebase.
Make the tests pass on all currently supported by this project versions of Go.
Done. CI is all green. PTAL, @gmlewis. |
|
Very nice, @shurcooL, thank you! |
|
Merged as 4 commits 64431e1...54b4d4b. |
Makes the new test pass. Fixes google#500. Closes google#501. Updates google#456.
Resolves #500.
This PR consists of 4 logical commits, please review by looking at each commit individually and read its commit message for description.
First commit is unrelated but got in my way, sorry.