Introducing review requests

54 points by jglovier 9 years ago | 15 comments

gkop 9 years 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.

piotrkaminski 9 years 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... :)

gkop 9 years 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.

deeplyoptional 9 years ago | |

Yes, outdated comments are hidden again.

gkop 9 years ago | | |

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

Kubuxu 9 years ago |

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

philips 9 years ago | |

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

donatj 9 years 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.

piotrkaminski 9 years 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.)

jastanton 9 years 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.

tomtheengineer 9 years 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-...

philips 9 years 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

jglovier 9 years ago | |

Great feedback. Thanks for sharing.

whyrusleeping 9 years 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.

piotrkaminski 9 years ago | |

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