Hacker News new | ask | show | jobs
by danpalmer 3751 days ago
Unfortunately that doesn't work for us for 2 reasons -

1. There's a data loss bug when multiple people edit the PR descriptions. I've had that as an open issue with GitHub for ~6 months, my team experiences it several times a week.

2. We use the checkboxes to assign and track reviewers, and since there's only one count of checkboxes, it would mess with our "2 of 5 complete" kind of metric for reviews.

I'd like first-class support for the concept of people reviewing and accepting (or rejecting) so that it's taken out of the PR description.

4 comments

You might be interested in taking a look at PullApprove (https://pullapprove.com/) -- basically you put a YAML in your repo that defines what code review looks like for your team (who, when, how many need to approve, etc. -- http://docs.pullapprove.com/). Approval/rejection can then be triggered by PR comments and it uses the status API so you know when the PR has passed review.

Code review "rules" can get pretty complicated since everyone works a little differently...not sure if GitHub will ever try to tackle that or not but we've got a lot of customers who are happy with how PullApprove fills the gap.

That looks great, I'll suggest that we start using it.
Folks who have this problem might want to check out Reviewable (https://reviewable.io):

1. Discussions are automatically treated as "issues" that need to be resolved before the review is complete. Resolving can be as simple as clicking "acknowledge" without following up, but there's also a rich system of "dispositions" that lets you block a discussion, resolve it unconditionally, etc.

2. You can set multiple assignees! (Only one will be reflected in GitHub, though.)

3. You can write a custom rule for determining review completion. One of the samples is "complete only when every assignee has sent an :lgtm:", giving the assignees explicit control over merge approval.

I know you've already decided against Reviewable danpalmer, but I figured others might still be interested. :)

Question: why do you use checklists to assign reviewers, when github has that functionality built in?
Because GitHub only allows you to assign to one person per issue.
Wait really? I could have sworn this wasn't true... Must have gotten confused somewhere
1) jbrooksuk's answer, only one assignee. 2) Assignees are the person owning the changes, not the person reviewing. 3) There is no status associated with it, you can't say "checked" vs "unchecked", and certainly not unreviewed/accepted/rejected.
This is exactly how voting, and reviewers concept works in RhodeCode. You have accept/reject/under review states for each reviewer, and you have to make full voting in order to merge a PR.