Skip to content

Add Support to preview endpoints to list branches or pull requests fo…#1186

Merged
gmlewis merged 1 commit intogoogle:masterfrom
sks444:list-branches-or-pull-requests-for-a-commit
Jun 10, 2019
Merged

Add Support to preview endpoints to list branches or pull requests fo…#1186
gmlewis merged 1 commit intogoogle:masterfrom
sks444:list-branches-or-pull-requests-for-a-commit

Conversation

@sks444
Copy link
Contributor

@sks444 sks444 commented Jun 8, 2019

…r a commit

Helps #1151

@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Jun 8, 2019
@sks444 sks444 force-pushed the list-branches-or-pull-requests-for-a-commit branch 2 times, most recently from a057467 to cd62ca4 Compare June 8, 2019 14:39
@codecov
Copy link

codecov bot commented Jun 8, 2019 •
edited
Loading

Codecov Report

Merging #1186 into master will decrease coverage by 0.03%.
The diff coverage is 61.53%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1186      +/-   ##
=========================================
- Coverage   70.24%   70.2%   -0.04%     
=========================================
  Files          84      84              
  Lines        5878    5904      +26     
=========================================
+ Hits         4129    4145      +16     
- Misses        958     963       +5     
- Partials      791     796       +5
Impacted Files Coverage Δ
github/github.go 87.53% <ø> (ø) ⬆️
github/pulls.go 71.42% <60%> (-1.24%) ⬇️
github/repos_commits.go 70.73% <63.63%> (-1.1%) ⬇️

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 6a35880...bc8872c. Read the comment docs.

gmlewis
gmlewis requested changes Jun 8, 2019
View reviewed changes
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Looks great, @sks444 !

Just a couple minor tweaks, please, and then we can merge once we get a second LGTM.

github/repos_commits.go Outdated
ListOptions
}

// BranchCommit is the result of listing branches with commit sha.
Copy link
Collaborator

@gmlewis gmlewis Jun 8, 2019

Choose a reason for hiding this comment

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

Let's please change this to SHA instead of sha.
(Also, please don't use force commits just to make things easier for reviewers.)

github/repos_commits.go Outdated
// ListBranchesHeadCommit gets all branches where the given commit SHA is the HEAD,
// or latest commit for the branch.
//
// Github API docs: https://developer.github.com/v3/repos/commits/#list-branches-for-head-commit
Copy link
Collaborator

@gmlewis gmlewis Jun 8, 2019

Choose a reason for hiding this comment

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

Please capitalize the H in GitHub.

@gmlewis gmlewis requested a review from gauntface June 8, 2019 17:58
@sks444
Copy link
Contributor Author

sks444 commented Jun 9, 2019

Thanks for the review @gmlewis, I have updated the PR.

gmlewis
gmlewis approved these changes Jun 9, 2019
View reviewed changes
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @sks444!

Awaiting second LGTM before merging.

vaibhavsingh97
vaibhavsingh97 reviewed Jun 10, 2019
View reviewed changes
github/repos_commits_test.go Outdated
client, mux, _, teardown := setup()
defer teardown()

// given
Copy link
Contributor

@vaibhavsingh97 vaibhavsingh97 Jun 10, 2019

Choose a reason for hiding this comment

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

Please don't add incomplete comment, can you please remove it?

vaibhavsingh97
vaibhavsingh97 reviewed Jun 10, 2019
View reviewed changes
github/pulls.go
// TODO: remove custom Accept header when this API fully launches.
acceptHeaders := []string{mediaTypeLabelDescriptionSearchPreview, mediaTypeLockReasonPreview, mediaTypeDraftPreview}
req.Header.Set("Accept", strings.Join(acceptHeaders, ", "))
var pulls []*PullRequest
Copy link
Contributor

@vaibhavsingh97 vaibhavsingh97 Jun 10, 2019

Choose a reason for hiding this comment

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

Please don't edit code which is not related to the issue 😄

vaibhavsingh97
vaibhavsingh97 reviewed Jun 10, 2019
View reviewed changes
Copy link
Contributor

@vaibhavsingh97 vaibhavsingh97 left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor change needed.
Good work @sks444 ! 🎉

@googlebot
Copy link

googlebot commented Jun 10, 2019

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes Indication that the PR author has signed a Google Contributor License Agreement. labels Jun 10, 2019
@sks444
Copy link
Contributor Author

sks444 commented Jun 10, 2019

Thanks @vaibhavsingh97, I have updated the pr according to your review.

@gmlewis, There was some conflicts so I did the rebase and did the push without using --force and this happened. Should I squash my commits into a single commit and do a force push or it is okay?

@gmlewis
Copy link
Collaborator

gmlewis commented Jun 10, 2019

@sks444 - sure, go ahead and do a force push since all the review feedback has already been addressed. That should hopefully clear it up.

@sks444 sks444 force-pushed the list-branches-or-pull-requests-for-a-commit branch from 1fb3a7b to bc8872c Compare June 10, 2019 19:06
@googlebot
Copy link

googlebot commented Jun 10, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Jun 10, 2019
@sks444
Copy link
Contributor Author

sks444 commented Jun 10, 2019

Thanks @gmlewis, it's done.

@gmlewis
Copy link
Collaborator

gmlewis commented Jun 10, 2019

Thank you, @sks444!
And thank you for the review, @vaibhavsingh97!

Merging.

@gmlewis gmlewis merged commit 361256a into google:master Jun 10, 2019
n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020
@sks444 @gmlewis
Add preview support to list branches or PRs for a commit (google#1186)
f94589e 
Fixes google#1151.
@mengmichael1 mengmichael1 mentioned this pull request Dec 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmlewis gmlewis gmlewis approved these changes

@gauntface gauntface Awaiting requested review from gauntface

+1 more reviewer

@vaibhavsingh97 vaibhavsingh97 vaibhavsingh97 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes Indication that the PR author has signed a Google Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sks444 @googlebot @gmlewis @vaibhavsingh97