3 ms·

The problem with pull requests / code reviews

19 points by philfreo 13y ago

birken 13y ago
At Thumbtack we use Github for our source control, but not our code review. We use rietveld -- https://code.google.com/p/rietveld/ https://code.google.com/p/rietveld/, which is an open source code review tool made to be similar to Google's internal code review tool, Mondrain. It has a much better view of the diff, incorporating much more context and a side-by-side view (here is a example change I just randomly pulled from rietveld's internal rietveld: https://codereview.appspot.com/8726052/diff/1/pom.xml https://codereview.appspot.com/8726052/diff/1/pom.xml). It also has the ability to group multiple in-line comments together in chunks, so it is apparent to coworkers when one round of the code review is done and they should work on it and get it back to you. It also has tons of other great features (email integration, keyboard shortcuts for moving around the code, etc etc). Of course, you can only use this if you control a project, and you will have to do a little integration work, but if code review is an important part of your team's workflow, I highly recommend the investment.
peter_l_downs 13y ago
If you're interesting in an alternative, I highly recommend Phabricator [0], a fantastic suite of tools that came out of Facebook. It has all sorts of random modules (memegen equivalents, etc.) but it has a really great feature tracker and a really great code review tool. Addressing your particular issue, it also shows the output of git diff, but includes buttons for '20 more lines of context' at the top and bottom of the diff [1]. Also, Evan Priestly (the lead contributor) responds immediately to any sort of issue and the source is readable. My workplace hosts our own, but I think there may be a hosted version available, too. [0] http://phabricator.org/ http://phabricator.org/ [1] http://imgur.com/BK55AKF http://imgur.com/BK55AKF
- ajtulloch 13y ago
  This is exactly right. Phabricator is the best code review tool I've ever tried, and it completely blows away GitHub, Google's Gerrit/Rietveld, etc. It's just an awesome experience using it, and huge thanks to Evan Priestly for creating it.
- drtse4 13y ago
  Another happy user of phabricator here, i introduced it as a single alternative to the usual set of tools we were using to track bugs, doing reviews, maintain a wiki, etc... and my coworkers seems to like it too. There are really a lot of additional modules (internal blogs, pastebin, images directory, polls and recently a web layout review tool) that are not really needed in a "traditional" setting, but the base functionalities are there and the tool can be adapted to different workflows. So, if someone is evaluating alternatives for an internal infrastructure, i'd give it a try. Edit: And in such "traditional" workplace, you'll likely need the option 'phabricator.serious-business' => true in your config file (it replaces button names with saner alternatives).
philfreo 13y ago
Someone pointed out that Bitbucket actually already has my "click '...' to expand" idea. Might be nicer with an animation, or if they started loading the AJAX request upon hover instead of click since it takes a while, but overall works well.
DanielRibeiro 13y ago
My biggest issue with pull requests was lack of syntax highlight. I managed to fix that though[1]. It would be nice to have it as a native feature though. [1] https://github.com/danielribeiro/github-diff-highlight#github-diff-highlight https://github.com/danielribeiro/github-diff-highlight#githu...

19 points by philfreo 13y ago

At Thumbtack we use Github for our source control, but not our code review. We use rietveld -- https://code.google.com/p/rietveld/ https://code.google.com/p/rietveld/, which is an open source code review tool made to be similar to Google's internal code review tool, Mondrain. It has a much better view of the diff, incorporating much more context and a side-by-side view (here is a example change I just randomly pulled from rietveld's internal rietveld: https://codereview.appspot.com/8726052/diff/1/pom.xml https://codereview.appspot.com/8726052/diff/1/pom.xml). It also has the ability to group multiple in-line comments together in chunks, so it is apparent to coworkers when one round of the code review is done and they should work on it and get it back to you. It also has tons of other great features (email integration, keyboard shortcuts for moving around the code, etc etc). Of course, you can only use this if you control a project, and you will have to do a little integration work, but if code review is an important part of your team's workflow, I highly recommend the investment.

peter_l_downs 13y ago

If you're interesting in an alternative, I highly recommend Phabricator [0], a fantastic suite of tools that came out of Facebook. It has all sorts of random modules (memegen equivalents, etc.) but it has a really great feature tracker and a really great code review tool. Addressing your particular issue, it also shows the output of git diff, but includes buttons for '20 more lines of context' at the top and bottom of the diff [1]. Also, Evan Priestly (the lead contributor) responds immediately to any sort of issue and the source is readable. My workplace hosts our own, but I think there may be a hosted version available, too. [0] http://phabricator.org/ http://phabricator.org/ [1] http://imgur.com/BK55AKF http://imgur.com/BK55AKF

ajtulloch 13y ago
This is exactly right. Phabricator is the best code review tool I've ever tried, and it completely blows away GitHub, Google's Gerrit/Rietveld, etc. It's just an awesome experience using it, and huge thanks to Evan Priestly for creating it.
drtse4 13y ago
Another happy user of phabricator here, i introduced it as a single alternative to the usual set of tools we were using to track bugs, doing reviews, maintain a wiki, etc... and my coworkers seems to like it too. There are really a lot of additional modules (internal blogs, pastebin, images directory, polls and recently a web layout review tool) that are not really needed in a "traditional" setting, but the base functionalities are there and the tool can be adapted to different workflows. So, if someone is evaluating alternatives for an internal infrastructure, i'd give it a try. Edit: And in such "traditional" workplace, you'll likely need the option 'phabricator.serious-business' => true in your config file (it replaces button names with saner alternatives).

philfreo 13y ago

Someone pointed out that Bitbucket actually already has my "click '...' to expand" idea. Might be nicer with an animation, or if they started loading the AJAX request upon hover instead of click since it takes a while, but overall works well.

DanielRibeiro 13y ago

My biggest issue with pull requests was lack of syntax highlight. I managed to fix that though[1]. It would be nice to have it as a native feature though. [1] https://github.com/danielribeiro/github-diff-highlight#github-diff-highlight https://github.com/danielribeiro/github-diff-highlight#githu...