Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

My team has benefited significantly from diligently reviewing pull requests. It may seem like a waste of time at first, but I can't count how many bugs we've caught this way. Team productivity is also increased due to knowledge-sharing, as we have to understand more parts of the code in order to properly review it. Reviews typically take between 10 mins and 4 hours, depending on the size of the PR. We've also improved our competence with Git, as we have tried to make the series of commits in a PR more understandable. PRs have actually increased work satisfaction on my team, as we get much more feedback on our work this way. Usually feedback is positive, but even when it isn't, we work at it together until the PR looks good.


If there are multiple releases/branches then I also recommend to very carefully review merges between branches, even files/regions where git (or whatever SCM is used) merged automatically with no conflicts. I've personally seen multiple incidents now were through review bugs and security issues where found in merges that ranged from subtle to catastrophic. I even saw one catastrophic security issue introduced into a file git merged with no conflicts.

(However, more issues are typically introduced by manual conflict resolution, since we humans are also easily confused when doing it. Both are a problem relatively independent of language, although some specific instances might be caught by a compiler or tests. If the latter don't catch it, it may be a sign that some tests are missing.)

Be careful.


I have a very similar experience. Another thing we do is to align our style guide (and automated linting) with code review, mostly with the intention of reducing diff noise and therefore mental overhead when reviewing. The ability to align in this way depends on the language, but we've had a fair bit of success doing this in Python.




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: