Skip to content

Implement Review Preview API - #860

Merged
tarebyte merged 4 commits into
octokit:masterfrom
soudy:add-review-preview
Feb 28, 2017
Merged

Implement Review Preview API#860
tarebyte merged 4 commits into
octokit:masterfrom
soudy:add-review-preview

Conversation

@soudy

@soudy soudy commented Jan 27, 2017

Copy link
Copy Markdown
Contributor

This PR implements the new reviews API https://developer.github.com/v3/pulls/reviews/.

@coveralls

coveralls commented Jan 27, 2017

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.09%) to 99.238% when pulling 6b4af23 on soudy:add-review-preview into 90eef39 on octokit:master.

@soudy
soudy force-pushed the add-review-preview branch 2 times, most recently from 5e58141 to a3bf9cf Compare January 27, 2017 14:14
@coveralls

coveralls commented Jan 27, 2017

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.07%) to 99.255% when pulling 5e58141 on soudy:add-review-preview into 90eef39 on octokit:master.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.07%) to 99.255% when pulling a3bf9cf on soudy:add-review-preview into 90eef39 on octokit:master.

2 similar comments
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.07%) to 99.255% when pulling a3bf9cf on soudy:add-review-preview into 90eef39 on octokit:master.

@coveralls

coveralls commented Jan 27, 2017

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.07%) to 99.255% when pulling a3bf9cf on soudy:add-review-preview into 90eef39 on octokit:master.

@soudy
soudy force-pushed the add-review-preview branch from a3bf9cf to 554a532 Compare January 27, 2017 14:31
@coveralls

coveralls commented Jan 27, 2017

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.02%) to 99.307% when pulling 554a532 on soudy:add-review-preview into 90eef39 on octokit:master.

@soudy
soudy force-pushed the add-review-preview branch from 554a532 to 60ff975 Compare January 27, 2017 14:40
@coveralls

coveralls commented Jan 27, 2017

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.02%) to 99.307% when pulling 60ff975 on soudy:add-review-preview into 90eef39 on octokit:master.

@soudy
soudy force-pushed the add-review-preview branch from 60ff975 to a43563c Compare January 27, 2017 15:21
@coveralls

coveralls commented Jan 27, 2017

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 99.342% when pulling a43563c on soudy:add-review-preview into 90eef39 on octokit:master.

@soudy
soudy force-pushed the add-review-preview branch from a43563c to 17f402a Compare January 27, 2017 16:53
@coveralls

coveralls commented Jan 27, 2017

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.2%) to 99.159% when pulling 17f402a on soudy:add-review-preview into 90eef39 on octokit:master.

@soudy
soudy force-pushed the add-review-preview branch from 17f402a to 654bd22 Compare January 29, 2017 17:58
@coveralls

coveralls commented Jan 29, 2017

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.05%) to 99.275% when pulling 654bd22 on soudy:add-review-preview into 90eef39 on octokit:master.

@soudy
soudy force-pushed the add-review-preview branch from 654bd22 to c519594 Compare January 30, 2017 16:49
@coveralls

coveralls commented Jan 30, 2017

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.8%) to 98.521% when pulling c519594 on soudy:add-review-preview into 90eef39 on octokit:master.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.09%) to 99.244% when pulling 756947f on soudy:add-review-preview into 90eef39 on octokit:master.

3 similar comments
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.09%) to 99.244% when pulling 756947f on soudy:add-review-preview into 90eef39 on octokit:master.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.09%) to 99.244% when pulling 756947f on soudy:add-review-preview into 90eef39 on octokit:master.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.09%) to 99.244% when pulling 756947f on soudy:add-review-preview into 90eef39 on octokit:master.

@soudy
soudy force-pushed the add-review-preview branch from 756947f to 29b9743 Compare February 1, 2017 14:55
@soudy

soudy commented Feb 1, 2017

Copy link
Copy Markdown
Contributor Author

I think this is ready for review, could someone take a look?

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 99.344% when pulling 29b9743 on soudy:add-review-preview into 90eef39 on octokit:master.

2 similar comments
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 99.344% when pulling 29b9743 on soudy:add-review-preview into 90eef39 on octokit:master.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 99.344% when pulling 29b9743 on soudy:add-review-preview into 90eef39 on octokit:master.

@soudy
soudy force-pushed the add-review-preview branch from 2ea870d to 73f5e99 Compare February 2, 2017 14:48
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 99.344% when pulling 73f5e99 on soudy:add-review-preview into 90eef39 on octokit:master.

2 similar comments
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 99.344% when pulling 73f5e99 on soudy:add-review-preview into 90eef39 on octokit:master.

@coveralls

coveralls commented Feb 2, 2017

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 99.344% when pulling 73f5e99 on soudy:add-review-preview into 90eef39 on octokit:master.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 99.344% when pulling 73f5e99 on soudy:add-review-preview into 90eef39 on octokit:master.

3 similar comments
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 99.344% when pulling 73f5e99 on soudy:add-review-preview into 90eef39 on octokit:master.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 99.344% when pulling 73f5e99 on soudy:add-review-preview into 90eef39 on octokit:master.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 99.344% when pulling 73f5e99 on soudy:add-review-preview into 90eef39 on octokit:master.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 99.344% when pulling f5a2506 on soudy:add-review-preview into 90eef39 on octokit:master.

3 similar comments
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 99.344% when pulling f5a2506 on soudy:add-review-preview into 90eef39 on octokit:master.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 99.344% when pulling f5a2506 on soudy:add-review-preview into 90eef39 on octokit:master.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 99.344% when pulling f5a2506 on soudy:add-review-preview into 90eef39 on octokit:master.

@soudy
soudy force-pushed the add-review-preview branch from f5a2506 to 9f177c8 Compare February 9, 2017 09:20
@coveralls

coveralls commented Feb 9, 2017

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 99.344% when pulling 9f177c8 on soudy:add-review-preview into 9051315 on octokit:master.

@soudy

soudy commented Feb 10, 2017

Copy link
Copy Markdown
Contributor Author

Anyone for a round 2? 😄

@tarebyte

Copy link
Copy Markdown
Contributor

I can take another look, @soudy.

If I might ask a favor though, would you mind not force pushing onto the branch anymore for this PR?

For someone who needs to review the work it's hard to keep track of what I'm reading when the code keeps constantly changing on "the same commit".

Please and thank you ✨

@soudy

soudy commented Feb 10, 2017

Copy link
Copy Markdown
Contributor Author

Ah sorry @tarebyte, will do. I do try to keep them organized but you're right about the changing.

@tarebyte tarebyte left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking good! I think we're almost there.

Comment thread lib/octokit/client/reviews.rb Outdated
# Get a single review
#
# @param repo [Integer, String, Hash, Repository] A GitHub repository
# @param pull_id [Integer] The id of the pull request

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we update all of the uses of pull_id to use number instead. It makes it consistent with the rest of the API methods.

Comment thread lib/octokit/client/reviews.rb Outdated
#
# @param repo [Integer, String, Hash, Repository] A GitHub repository
# @param pull_id [Integer] The id of the pull request
# @param review_id [Integer] The id of the review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we also update all uses of review_id to be review. This makes it more consistent with the rest of the library.

@coveralls

coveralls commented Feb 28, 2017

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 99.345% when pulling 756a1c4 on soudy:add-review-preview into 9051315 on octokit:master.

@pulusanidamini

Copy link
Copy Markdown

Hi @soudy .. Any idea when this PR will be merged ?

@soudy

soudy commented Feb 28, 2017

Copy link
Copy Markdown
Contributor Author

@pulusanidamini I just implemented @tarebyte's last feedback, should be good to go now I think. 😄

@tarebyte

Copy link
Copy Markdown
Contributor

Thanks for working on this @soudy 💖

@tarebyte
tarebyte merged commit b0b8fb7 into octokit:master Feb 28, 2017
@soudy
soudy deleted the add-review-preview branch February 28, 2017 21:59
@bbuchalter

Copy link
Copy Markdown

I was just trying this out but did not get the results I expected. See attached screenshots.
screen shot 2017-03-04 at 1 23 29 am
screen shot 2017-03-04 at 1 23 06 am

Please let me know if I can provide any other detail, or if you prefer a different forum for this report. Thanks for your work on octokit!

@bbuchalter

Copy link
Copy Markdown

Upon further investigation, the PR I was testing with was merged and the requested review had been completed. Perhaps some documentation about what states the requests must be in to be returned in the API would be helpful?

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.

5 participants