Introducing review requests (opens in new tab)

(github.com)

54 pointsjglovier9y ago15 comments

15 comments

gkop9y ago

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.

piotrkaminski9y ago

Heh, I never cease to wonder at the variety of -- often contradictory -- code review workflows. For me and many others automatically disappearing comments are a misfeature, since we _want_ to verify that a comment was correctly addressed. To each their own... :)

gkop9y ago

At the launch of GitHub reviews, it's not just that the outdated comments weren't hidden, but at that time, there were no tools in place of that to collapse threads, mark sections of diff as approved, or otherwise resolve comments with follow-on pushes. That's why I asked their support if I was using it correctly, I was super confused. I'm definitely interested in a better GitHub workflow.

deeplyoptional9y ago

Yes, outdated comments are hidden again.

gkop9y ago

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

Kubuxu9y ago

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

philips9y ago

This is one of the features I love about https://reviewable.io.

donatj9y ago

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.

piotrkaminski9y ago

FWIW https://reviewable.io supports arbitrarily complex review completion requirements, and the outcome gets fed into a PR status that you can make required as well. Right now the completion condition must be self-contained but I'm considering passing in designated metadata files so you could have, e.g., per-directory "owners" like Google does.

(Disclosure: I'm the founder.)

jastanton9y ago

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.

tomtheengineer9y ago

+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-...

philips9y ago

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

jglovierOP9y ago

Great feedback. Thanks for sharing.

whyrusleeping9y ago

Are there any api endpoints now for querying reviews? I'd love to have a way to get a list of reviews i'm being asked to do.

piotrkaminski9y ago

AFAICT the PR review REST API is still on their "near-term roadmap" (https://developer.github.com/early-access/platform-roadmap/).

j / k navigate · click thread line to collapse

15 comments

gkop9y ago

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

piotrkaminski9y ago

gkop9y ago

deeplyoptional9y ago

Yes, outdated comments are hidden again.

gkop9y ago

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

Kubuxu9y ago

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

philips9y ago

This is one of the features I love about https://reviewable.io.

donatj9y ago

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.

piotrkaminski9y ago

(Disclosure: I'm the founder.)

jastanton9y ago

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.

tomtheengineer9y ago

+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-...

philips9y ago

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

jglovierOP9y ago

Great feedback. Thanks for sharing.

whyrusleeping9y ago

Are there any api endpoints now for querying reviews? I'd love to have a way to get a list of reviews i'm being asked to do.

piotrkaminski9y ago

AFAICT the PR review REST API is still on their "near-term roadmap" (https://developer.github.com/early-access/platform-roadmap/).

j / k navigate · click thread line to collapse