The Merge methods API is now official #562
Conversation
see: https://developer.github.com/changes/2017-04-07-end-merge-methods-api-preview/ this removes the apiVersion (that wasn't being used anyway..) also adds support for rebase mergeMethod per https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button Since this previously looked for the value to be bool, this is a breaking change
|
I could technically update this PR to not be breaking, just seems really weird to me to randomly have a mixed string/bool value used here |
It is weird, I do agree with you. But we should keep BC. Though, we should document this to make sure we fix it before next major release. |
lib/Github/Api/PullRequest.php
Outdated
| @@ -163,6 +158,10 @@ public function merge($username, $repository, $id, $message, $sha, $mergeMethod | |||
| $mergeMethod = $mergeMethod ? 'squash' : 'merge'; | |||
| } | |||
|
|
|||
| if (!in_array($mergeMethod, array('squash', 'rebase'))) { | |||
There was a problem hiding this comment.
👎 We should let GitHub's API throw an error.
There was a problem hiding this comment.
That's fine but there are several other places throughout the codebase that behave in this way already, just do a search for in_array to see what I mean. Should an issue be opened to remove all of those checks in the next major version for consistency?
|
@Nyholm @GrahamCampbell just checking back in on this to see if you do want me to make the requested change (even though plenty of other places in the codebase already follow that pattern). |
|
I think it should be changed here, regardless of the other classes. |
|
Otherwise, someone could write |
|
Thank you both. I did not reflect on this. I've done some digging: @bobeagan you are correct, that is the pattern we usually do. However, we do it for reading data. When we write we throw an exception. See https://github.com/KnpLabs/php-github-api/blob/2.2.0/lib/Github/Api/PullRequest/Review.php#L120 I think we should throw exception even for reading data, but that is BC... (I'll written an issue (#566)) I think you should throw an exception here. |
lib/Github/Api/PullRequest.php
Outdated
| * | ||
| * @return self | ||
| */ | ||
| public function configure($bodyType = null, $apiVersion = null) |
There was a problem hiding this comment.
Even though removing this is not breaking BC, I think we should keep it. Because if we remove it and then later adds another optional parameter to this function, that WILL be BC and it is really hard to keep track of.
There was a problem hiding this comment.
In that case I'll need to fix it since I realized when I was removing it that I never actually used the $apiVersion variable when setting the accept header value. :(
…in the future there are no preview versions now so checking against an empty array also had to fix the acceptHeaderValue to correctly use the variable
|
@Nyholm and @GrahamCampbell updated and ready for another look |
|
Thank you |
see: https://developer.github.com/changes/2017-04-07-end-merge-methods-api-preview/
this removes the apiVersion (that wasn't being used anyway..)
also adds support for rebase mergeMethod per https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button