Hacker News Reader

Comment by danpalmer | Hacker News Reader

This is a great step in the right direction. I use GitHub for code review every day, and it has historically been very poorly designed for thorough reviews. These changes look great, I just hope that we get some sort of checking-off of review points, and accept/reject functionality.

View on HN

I'd love a UI way to enable `?w=1` whitespace mode, especially for html/json/yaml files. I have a feeling a lot of reviewers don't know about it and spend an inordinate amount of time squinting to see what changes when it was a whitespace-only change.

This! ?w=1 has existed for years, with no UI way to access it.

Compare two views of the same commit (found this by searching for any commit on github with the word "indent" in it)

https://github.com/douglascrockford/JSON-js/commit/1e3869cb3... (133 additions and 127 deletions)

https://github.com/douglascrockford/JSON-js/commit/1e3869cb3... (30 additions and 24 deletions)

On top of that.. when you're in `?w=1` view you can't leave line-by-line comments. :facepalm:

I have a userscript that does just that, however it needs to be updated to account for these new review tools.

https://gist.github.com/xPaw/de6ee132a2e267ef6960

Some similar option for enabling patience diffing as well would be amazing. Those two options in combination would certainly make a not insignificant amount of PRs much more readable.

I created a reminder for myself so that every six months I write an email to GitHub asking for someone to create a user option for this whitespace setting.

I REALLY wish I could exclude certain file types from the view.

I wonder if there's a set of API hooks they could expose to allow third party applications to handle more advanced code review techniques? I would totally understand GitHub not wanting to implement a full Phabricator/Gerrit review feature set, but maybe they could make it easier to integrate other services for it.

That's a great thought. For my own edification, which third party apps come to mind?

At present, the "official" collaboration tools in the GitHub integrations directory are here: https://github.com/integrations/feature/collaborate

I don't understand it :)

GitHub has become the de-facto place to host open source projects. Improving the tooling around code merging, seems like a no-brainer, and a huge potential win for the projects using it, especially the larger ones.

I agree. I think there's a whole ecosystem there.

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.

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.

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.

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