Introducing review requests

#1

Post by jglovier » Wed, Dec 07, 2016, 10:12 PM UTC

Re: Introducing review requests

#2

Post by donatj » Wed, Dec 07, 2016, 10:25 PM UTC

Kind of interesting. What I'd really like to be able to do is require specific people to review before merging, for example require a review by at least one of our senior developers.

Right now the greenest guy on the team can approve a PR and it can be merged.

The real dream would be to be able to set the requirements on a per-directory basis, but I doubt that would ever happen.

Re: Introducing review requests

#3

Post by Kubuxu » Wed, Dec 07, 2016, 10:36 PM UTC

Being able to search for things my review is requested on would be good companion feature for it.

Re: Introducing review requests

#4

Post by jglovier » Wed, Dec 07, 2016, 10:47 PM UTC

donatj wrote: post #2

Kind of interesting. What I'd really like to be able to do is require specific people to review before merging, for example require a review by at least one of our senior developers. Right now the greenest guy on the team can approve a PR and it can be merged. The real dream would be to be able to set the requirements on a per-directory basis, but I doubt that would ever happen.

Great feedback. Thanks for sharing.

Re: Introducing review requests

#5

Post by jastanton » Wed, Dec 07, 2016, 11:17 PM UTC

donatj wrote: post #2

Kind of interesting. What I'd really like to be able to do is require specific people to review before merging, for example require a review by at least one of our senior developers. Right now the greenest guy on the team can approve a PR and it can be merged. The real dream would be to be able to set the requirements on a per-directory basis, but I doubt that would ever happen.

I second this. I am an xoogler and we had a file inside of folders that dictated who you needed to get approval by. When someone edited code the review tool would crawl the tree to find the nearest owners file. We couldn't merge unless someone from that file approved the change request. Worked great!

Now that we have this tool I wonder if there is an API to add a reviewer. It would be fairly trivial to add this if there was an API.

Re: Introducing review requests

#6

Post by tomtheengineer » Wed, Dec 07, 2016, 11:25 PM UTC

jastanton wrote: post #5

donatj wrote: post #2

Kind of interesting. What I'd really like to be able to do is require specific people to review before merging, for example require a review by at least one of our senior developers. Right now the greenest guy on the team can approve a PR and it can be merged. The real dream would be to be able to set the requirements on a per-directory basis, but I doubt that would ever happen.

I second this. I am an xoogler and we had a file inside of folders that dictated who you needed to get approval by. When someone edited code the review tool would crawl the tree to find the nearest owners file. We couldn't merge unless someone from that file approved the change request. Worked great! Now that we have this tool I wonder if there is an API to add a reviewer. It would be fairly trivial to add this if th…

+1. Gerrit has a similar way to express access permissions that differentiate between a "contributor" and a "developer": https://gerrit-review.googlesource.com/Documentation/access-...

Re: Introducing review requests

#7

Post by gkop » Wed, Dec 07, 2016, 11:33 PM UTC

Has GitHub improved reviews since launch? I tried the review feature at launch and found it to be a severe regression from plain PR inline comments, because the review comments weren't hidden when the code they referenced changed, so I had to manually check each review comment to see whether or not it was addressed by a follow-on commit. I emailed their support asking if I was using the review feature wrong, and they said No, I was using it correctly, and it was a known weakness in the feature that they were tracking.

Review requests are useless until GitHub makes the reviews themselves useful.

Re: Introducing review requests

#8

Post by deeplyoptional » Wed, Dec 07, 2016, 11:36 PM UTC

gkop wrote: post #7

Has GitHub improved reviews since launch? I tried the review feature at launch and found it to be a severe regression from plain PR inline comments, because the review comments weren't hidden when the code they referenced changed, so I had to manually check each review comment to see whether or not it was addressed by a follow-on commit. I emailed their support asking if I was using the review feature wrong, and they…

Yes, outdated comments are hidden again.

Re: Introducing review requests

#9

Post by gkop » Wed, Dec 07, 2016, 11:37 PM UTC

deeplyoptional wrote: post #8

gkop wrote: post #7

Has GitHub improved reviews since launch? I tried the review feature at launch and found it to be a severe regression from plain PR inline comments, because the review comments weren't hidden when the code they referenced changed, so I had to manually check each review comment to see whether or not it was addressed by a follow-on commit. I emailed their support asking if I was using the review feature wrong, and they…

Yes, outdated comments are hidden again.

Thanks! I look forward to taking reviews for another test drive soon.

Re: Introducing review requests

#10

Post by philips » Wed, Dec 07, 2016, 11:50 PM UTC

donatj wrote: post #2

Kind of interesting. What I'd really like to be able to do is require specific people to review before merging, for example require a review by at least one of our senior developers. Right now the greenest guy on the team can approve a PR and it can be merged. The real dream would be to be able to set the requirements on a per-directory basis, but I doubt that would ever happen.

Yes, this is important. The Kubernetes community is having to build bots that enforce this sort of workflow.

https://github.com/kubernetes/contrib/pull/2076