Protection configure #568
Protection configure #568
Conversation
lib/Github/Api/PullRequest.php
Outdated
| @@ -163,6 +164,10 @@ public function merge($username, $repository, $id, $message, $sha, $mergeMethod | |||
| $mergeMethod = $mergeMethod ? 'squash' : 'merge'; | |||
| } | |||
|
|
|||
| if (!in_array($mergeMethod, array('merge', 'squash', 'rebase'), true)) { | |||
| throw new InvalidArgumentException(sprintf('"$mergeMethod" must be one of ["merge", "squash", "rebase"] ("%s" given).', $mergeMethod)); | |||
There was a problem hiding this comment.
Seems silly to hard code this. Let GitHub's API decide what is or isn't a valid option.
There was a problem hiding this comment.
Well, my comment still stands.
There was a problem hiding this comment.
It is better to fail early. Since the API client knows that the request is going to fail, why now throw an exception right away?
There was a problem hiding this comment.
GitHub reserve the right to add new merge methods, so the client does not know this.
There was a problem hiding this comment.
We argue about how smart/helpful or ("helpful") the client should be.
My opinion is not more valid than any other, but will I approve this because of consistency. We throw InvalidArugmentException when we know the API is going to return a fail in Review::create, CurrentUser/Emails::add, Issue/Labels::add. And as mentioned before; on plenty of places we silently correct wrong input parameters.
|
This PR only adds a traits and configure method to the Protection API. The diff is unfair. Try to not put unrelated commits into PRs in the future. |
|
You have to rebase this PR and solve the conflicts. |
bobeagan
force-pushed
the
protection-configure
branch
from
6f5577d to
adecb25
Compare
|
@Nyholm sorry about that, i hoped github would magically handle that for me when the other branch got merged, it has been rebased |
Add missing loki-preview accept header for branch protection methods
This needs to wait for #562 to merge because I am a
badlazy person