Added dismissal review mandatory "message" parameter #606
Conversation
Nyholm
left a comment
Nyholm
left a comment
There was a problem hiding this comment.
Thank you for this PR. Could I get a second opinion about this being a BC break or not?
| * @return array|string | ||
| */ | ||
| public function dismiss($username, $repository, $pullRequest, $id) | ||
| public function dismiss($username, $repository, $pullRequest, $id, $message) |
There was a problem hiding this comment.
This is technically a BC break. But i think we should consider it as a bugfix since the method as has been unusable before this PR.
There was a problem hiding this comment.
Yeah, I was getting a 500 Internal Error due to the missing required parameter $message :)
There was a problem hiding this comment.
Did you get 500 from the API server?
There was a problem hiding this comment.
If the message is not provided it seem that Github does not like it very well. I was expecting something like a 422 Error, but I got a 500 instead (A ruby nil error seem to be uncaught by Github).
There was a problem hiding this comment.
I was also in doubt if this was a BC break but indeed the message is required. It could be this changed in the preview period. So this should be documented but it's a correct change. See https://developer.github.com/v3/pulls/reviews/#input-2
There was a problem hiding this comment.
👍 I would also report this to github so they can fix their error in this case!
There was a problem hiding this comment.
I already did it :)
There was a problem hiding this comment.
My mistake, it's not always a 500 error, it look completely random, sometimes I got a 400, sometimes a 422 O.O
There was a problem hiding this comment.
That's possibly a github error, but this PR is right. The message is required, so we should change the method
There was a problem hiding this comment.
@acrobat Let's add your review, so we can merge this asap (if you agree w/ this one).
Thanks :)
|
Thanks @Geolim4! |
|
Good work! |
|
FYI: @acrobat we should consider "squash" PRs when merging. So each PR get one commit in the history. |
|
There's no doc about reviews ? If not can I add one (based on the other docs) ? |
|
@Nyholm yes indeed you are right! Should we just do it with the github merge button or do you have tool for that? (something like fabpot's mergetool) |
|
@Geolim4 It looks like the docs are indeed missing! If you have the time please open a PR for the missing docs, thanks! :) |
|
Just do it with the github merge button. |
|
@acrobat PR sent :) |
3 participants
No description provided.