Skip to content

Not differentiating between blank string and null causes difficulties in merging pull requests #1815

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

Closed
john-m-liu opened this issue Mar 2, 2021 · 4 comments · Fixed by #1816

Comments

@john-m-liu
Copy link
Contributor

john-m-liu commented Mar 2, 2021

(Github API doc is at https://docs.github.com/en/rest/reference/pulls#merge-a-pull-request)

When making a request to merge a pull request, there is a difference between the following:
curl -X PUT -H "Authorization: <token>" https://github.com/api/repos/{owner}/{repo}/pulls/{pull_number}/merge -d '{"merge_method": "squash", "commit_title": "my commit", "commit_message": ""}'
and
curl -X PUT -H "Authorization: <token>" https://github.com/api/repos/{owner}/{repo}/pulls/{pull_number}/merge -d '{"merge_method": "squash", "commit_title": "my commit"}'
Specifically, the former will have no commit message, and the latter will have the default commit message (typically the concatenation of all commit messages in the pull request).

This differentiation isn't possible with the go-github library because the following struct has omitempty: https://github.com/google/go-github/blob/master/github/pulls.go#L435. The default commit message will always be used if you pass the blank string.

I believe this is a specific case of #19 but was not addressed

@gmlewis
Copy link
Collaborator

gmlewis commented Mar 2, 2021

One non-breaking-API-change solution would be to add a new endpoint, something like MergeWithNoCommitMessage or something like that.

Any other solution I can think of would involve a breaking API change, and additionally be rather ugly... like adding another parameter stating whether CommitMessage should be sent or not.

I'm leaning toward the former, but am not against the latter.

Thoughts?

@john-m-liu
Copy link
Contributor Author

One non-breaking-API-change solution would be to add a new endpoint, something like MergeWithNoCommitMessage or something like that.

Any other solution I can think of would involve a breaking API change, and additionally be rather ugly... like adding another parameter stating whether CommitMessage should be sent or not.

I'm leaning toward the former, but am not against the latter.

Thoughts?

The latter feels a bit cleaner to me simply because it would avoid 2 public functions that do basically the same thing, but I don't really have a preference either way. I believe you could make the second option non-breaking by having the new parameter be in a struct field where false maintains the existing behavior

@gmlewis
Copy link
Collaborator

gmlewis commented Mar 2, 2021

The latter feels a bit cleaner to me simply because it would avoid 2 public functions that do basically the same thing, but I don't really have a preference either way. I believe you could make the second option non-breaking by having the new parameter be in a struct field where false maintains the existing behavior

You are right... I just noticed that we are already passing options *PullRequestOptions to the Merge method, so we really could make it completely non-breaking just as you said.

This definitely sounds like the best option.

Would you like to work on this, @john-m-liu ? Or do you want me to open up this issue to our other volunteer contributors?

@john-m-liu
Copy link
Contributor Author

Sure, I can submit a PR shortly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants