Hacker News Reader

Comment by faitswulff | Hacker News Reader

It's interesting that you mention checking off of review points. I had begun to do that in an informal fashion by updating my pull request with notes in the form of a checklist:

- [x] Refactor _

- [ ] Rename variable x

- [ ] ...

Now that you mention it, it would be very nice to have something like Google Docs's ability to mark comments as resolved.

View on HN

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.

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.

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.

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. :)

This is always a deal breaker for thorough code reviews - it's always super hard to make sure that everything got resolved in a later commit. The only tool I've seen handle this well is SmartBear's Code Collaborator - you mark certain comments as defects, it does a decent job keeping them aligned with their context as revisions happen afterwards, and then you can close them out before marking the review LGTM. Unfortunately, Code Collaborator is horrible at basically everything else. It would be nice to have an easy-to-use todo list that's easy to close out and not lose context.

Bitbucket does this too.

Yep, specifically Bitbucket allows you to create tasks from comments. Bitbucket Server also (optionally) prevents the pull request from being merged until all tasks have been resolved.

Thanks! I'll have to check this out. I've used the Stash equivalent of this and it was clunky, requiring something like 3 steps to make a new task.

Reply on news.ycombinator.com