Go-github: Add support for changing base branch on pull requests

[willnorris]

announcement: https://developer.github.com/changes/2016-08-23-change-base/

This is going to be problematic since PullRequestsService.Edit takes a PullRequest whose current base field is of type PullRequestBranch.

[parkr]

This is going to be problematic since PullRequestsService.Edit takes a PullRequest whose current base field is of type PullRequestBranch.

I might be way off base, but is PullRequestsService.EditBase a no-go?

[willnorris]

I might be way off base

I see what you did there :smirk:

The other option is to add state to NewPullRequest, and change PullRequestsService.Edit to take that as a parameter. Which is a breaking change and also really ugly since the name NewPullRequest doesn't make sense in that context, and it means if you already have a PullRequest object you can't simply pass it to the Edit method, you'd have to convert it first.

I gotta say, design decisions like this in the GitHub API drive me absolutely insane. it would have been a simple matter to have the payload for updating a pull request be:

{
  "title": "Amazing new feature",
  "body": "Please pull this in!",
  "head": "octocat:new-feature",
  "base": {
    "ref: "master"
  }
}

And then it would have been compatible with the existing Pull Request JSON object. Come to think of it, why doesn't GitHub simply accept both styles? (Maybe it does? I haven't tried it)

[dmitshur]

I gotta say, design decisions like this in the GitHub API drive me absolutely insane. it would have been a simple matter to have the payload for updating a pull request be:
...
And then it would have been compatible with the existing Pull Request JSON object. Come to think of it, why doesn't GitHub simply accept both styles? (Maybe it does? I haven't tried it)

If you think so, it might be worth to send [email protected] an email with that question/suggestion. In my experience, they do a good job of either elaborating on their rationale for an API decision, or taking feedback into account and improving it. At least while the API is in preview period and they're purposefully asking for feedback on it:

If you have any questions or feedback, please let us know!

[lucasalcantara]

How this issue going? Is it still have to be done?

[lucasalcantara]

@willnorris @shurcooL is this still have to be done?

[dmitshur]

As far as I can tell, this issue is unresolved and needs to be done. Latest status appears to be https://github.com/google/go-github/issues/421#issuecomment-242102052.

[gmlewis]

I've sent a request to GitHub Tech Support and will update this thread with their response.

[gmlewis]

I had an exchange with the excellent GitHub Tech Support team, and they pointed out that the PATCH API to update a PR never supported the sending of a JSON object representing a PullRequest, which may or may not be an obvious statement to others reading this thread.

I did some digging, and that is indeed the case.

Unfortunately, they are not amenable to supporting the use case of the above recommendation.

My recommendation is that we break the go-github API and make a new opt struct for the Edit method to pass in the optional fields (which is a common pattern in this package), using omitempty as usual.

Thoughts?

[willnorris]

what about leaving pulls.Edit as it is today, and have it continue taking a PullRequest as input, but internally it converts it to a

type pullRequestUpdate struct {
    Title *string `json:"title,omitempty"`
    Body  *string `json:"body,omitempty"`
    State  *string `json:"state,omitempty"`
    Base  *string `json:"base,omitempty"`
} 

or whatever we name it. Basically, just copy over Title, Body, and State as-is, and copy Base from the passed-in PullRequest.Base.Ref. This is also a pattern we already have in the library where we occasionally use unexported types for construct requests.

[dmitshur]

I had an exchange with the excellent GitHub Tech Support team

Thank you for doing that work @gmlewis!

My recommendation is that we break the go-github API and make a new opt struct for the Edit method to pass in the optional fields (which is a common pattern in this package), using omitempty as usual.

So something like PullRequestEdit, a struct dedicated to editing a pull request, right?

I'd be in agreement with doing that. We often start out trying to reuse the same struct for get and edit operations, but later find out they're not really compatible (e.g., when editing a pull request, most fields of PullRequest such as ID, Number, CreatedAt, UpdatedAt, DiffURL, etc., are not applicable).

@willnorris's suggestion works too. We can do that first, for now, since it's less expensive (no public API change), and we can consider/defer the PullRequestEdit idea until later, when using PullRequest for editing becomes too unwieldy.

[gmlewis]

I agree. Let's do @willnorris' suggestion above.
This should also be relatively straightforward, so I'm going to mark this issue as "good for beginner" in case anyone else wants to contribute.

[sahildua2305]

I'm not sure if I understand @willnorris's suggestion completely. But I'd like to work on this. 🙂

[dmitshur]

@sahildua2305, an example of what I think his suggestion was is similar to createCommit here. See how CreateCommit method takes commit *Commit, but internally uses createCommit.

[willnorris]

@sahildua2305, an example of what I think his suggestion was is similar to createCommit here. See how CreateCommit method takes commit *Commit, but internally uses createCommit.

Yes, that was the example I was thinking about.