Skip to content
This repository was archived by the owner on Apr 20, 2026. It is now read-only.

Add Request Review functions (list, create, remove)#36

Merged
henriquetruta merged 2 commits into
spotify:masterfrom
vglocus:master
Sep 17, 2020
Merged

Add Request Review functions (list, create, remove)#36
henriquetruta merged 2 commits into
spotify:masterfrom
vglocus:master

Conversation

@vglocus
Copy link
Copy Markdown
Contributor

@vglocus vglocus commented Sep 15, 2020

Extend PullRequestClient with methods for listing, creating and removing requested reviews.

Can reach out using viktorg@spotify.com

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 15, 2020

Codecov Report

Merging #36 into master will increase coverage by 2.15%.
The diff coverage is 80.95%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #36      +/-   ##
============================================
+ Coverage     65.80%   67.95%   +2.15%     
- Complexity      176      188      +12     
============================================
  Files            30       32       +2     
  Lines           731      752      +21     
  Branches         29       29              
============================================
+ Hits            481      511      +30     
+ Misses          229      220       -9     
  Partials         21       21              
Impacted Files Coverage Δ Complexity Δ
...m/spotify/github/v3/clients/PullRequestClient.java 26.41% <63.63%> (+26.41%) 5.00 <2.00> (+5.00)
...va/com/spotify/github/v3/clients/GitHubClient.java 72.72% <100.00%> (+3.57%) 45.00 <4.00> (+4.00)
...spotify/github/v3/prs/RequestReviewParameters.java 100.00% <100.00%> (ø) 1.00 <1.00> (?)
.../java/com/spotify/github/v3/prs/ReviewComment.java 100.00% <0.00%> (ø) 1.00% <0.00%> (?%)
...va/com/spotify/github/v3/prs/ReviewParameters.java 100.00% <0.00%> (+100.00%) 1.00% <0.00%> (+1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef7fc68...a68a34d. Read the comment docs.

@vglocus
Copy link
Copy Markdown
Contributor Author

vglocus commented Sep 15, 2020

@henriquetruta

Copy link
Copy Markdown
Contributor

@henriquetruta henriquetruta left a comment

Choose a reason for hiding this comment

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

Thanks for your PR! A few suggestions inline

public CompletableFuture<Void> removeRequestedReview(final int number, final RequestReviewParameters properties) {
final String path = String.format(PR_REVIEW_REQUESTS_TEMPLATE, owner, repo, number);
final String jsonPayload = github.json().toJsonUnchecked(properties);
log.debug("Requesting reviews for PR: " + path);
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.

this comment seems to have been copied from the one above

log.debug("Fetching pull requests from " + path);
return github.request(path, LIST_PR_TYPE_REFERENCE);
}

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.

minor: unnecessary new lines

* @param properties properties for reviewing the PR, such as reviewers and team_reviewers.
* @see "https://docs.github.com/en/rest/reference/pulls#request-reviewers-for-a-pull-request"
*/
public CompletableFuture<Void> removeRequestedReview(final int number, final RequestReviewParameters properties) {
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.

it'd be nice to add a test for this case, so we can get the new top-level delete(path, jsonPayload) method tested as well

@vglocus
Copy link
Copy Markdown
Contributor Author

vglocus commented Sep 17, 2020

Thank you @henriquetruta. I've fixed your comments and extended the testing somewhat.

Copy link
Copy Markdown
Contributor

@henriquetruta henriquetruta left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments!

@henriquetruta henriquetruta merged commit f862545 into spotify:master Sep 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants