Skip to content
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

Add support for review requests in pull-request command #1453

Merged
merged 7 commits into from
Apr 25, 2017

Conversation

tomasv
Copy link
Contributor

@tomasv tomasv commented Apr 17, 2017

Fixes #1358

I took the code from #1422 and attempted to complete it. I hope @ConradIrwin does not mind. 👼

I'm not sure what the cleanest way of overriding Accept for one request would be. Most options involve adding the apiPayloadVersion argument to a lot of function signatures and passing it through. Can this be improved?

@ConradIrwin
Copy link

Thanks @tomasv !

@hkdobrev
Copy link

Thanks for this! However, the title is a bit misleading as this adds support for only adding reviewers, not submitting reviews. I think it should be renamed similar to the issue it closes.

This allows us to access extra functionality around PR reviews.
Previously, only the first value was shown.
@mislav
Copy link
Owner

mislav commented Apr 24, 2017

I've pushed some cleanups to this. Instead of setting a changed Accept header per single request, I've simply added the Accept: application/vnd.github.black-cat-preview+json value to all requests. This should have no adverse effect to the rest of the API functionality, and is only recognized by API endpoints that deal with reviewer functionality. Let me know what you think!

Copy link
Contributor Author

@tomasv tomasv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mislav looks good to me.

From the previous PR attempt I assumed it was important to have a different Accept header just for this one request, but if that assumption is not valid then this is much simpler. :)

@mislav
Copy link
Owner

mislav commented Apr 24, 2017

Hmm, I've just tested sending the 2 header values to GitHub API, but it doesn't recognize it:

> POST https://api.github.com/repos/mislav/playground/pulls/30/requested_reviewers
> Authorization: token [REDACTED]
> Accept: application/vnd.github.v3+json;charset=utf-8
> Accept: application/vnd.github.black-cat-preview+json
{"reviewers":["mislav"]}

< HTTP 415
{"message":"If you would like to help us test the Pull Request Reviews API during its preview period, you must specify a custom media type in the 'Accept' header. Please see the docs for full details.","documentation_url":"https://developer.github.com/v3"}

It's possible that GitHub API doesn't deal well with multiple values of Accept header, even though it should in theory. I will follow up to see whether this is a bug in our API.

@tomasv
Copy link
Contributor Author

tomasv commented Apr 24, 2017

@mislav is that an accurate representation of header values?

The final raw request header as I understand should look like this: Accept: application/vnd.github.v3+json;charset=utf-8, application/vnd.github.black-cat-preview+json. Relevant RFC: https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html

@mislav
Copy link
Owner

mislav commented Apr 24, 2017

@tomasv Specifying a header twice or specifying it once with two values separated by a comma should be equivalent. But, I'm just checking the implementation of this in the GitHub API, and it looks like we only take the 1st value of Accept and discard the rest. I'll check with the API team to see whether this was desired behavior or an oversight.

In the meantime, I'll try switching back to a custom Accept header just for this request.

The GitHub API currently doesn't recognize multiple values of `Accept`
header, so ensure that `black-cat-preview` is the main one when
requesting reviewers.
@mislav mislav merged commit f4930b5 into mislav:master Apr 25, 2017
@evnm
Copy link

evnm commented Apr 25, 2017

Thank you for adding this! Will it be included in 2.3.0?

@mislav
Copy link
Owner

mislav commented Apr 26, 2017

Yes, and I'll try to make a prerelease soon including this.

@AlexVKO
Copy link

AlexVKO commented Jun 19, 2017

hey, Is it currently working?

@mislav
Copy link
Owner

mislav commented Jun 19, 2017

@AlexVKO You can try this functionality out if you build hub from the master branch. It's not yet released.

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 this pull request may close these issues.

6 participants