The goal of this issue is to discuss and possibly introduce a new process when developing code on the Piwik git repository: introducing Pre-merge code reviews.

The Rules

1. Piwik core and all plugins
   The Rules apply to this piwik/piwik repository as well as all plugin-* repositories in piwik and PiwikPRO Github organizations.
2. Pushing to master is restricted
   For minor changes (typos, small bugs, trivial features), you are allowed to push to master directly. For any large change, always push to a separate branch per logical unit (story, feature, bug, optimisation, refactor, improvement).
3. Never Merge Your Own Branch
   This helps to ensures that code is in fact reviewed.
4. Review Work in Progress First
   When you are finished with a task, you notify the other team members that your work is ready for final review. Then you review existing branches. Before picking up a new task, you look at all open pull requests (including unfinished ones) and review the changes since the last time you checked.
5. Merge responsibly
   Merging a pull request is the responsibility of the whole team. A pull request can not be merged when someone in the team does not understand the code or the reasoning, or does not agree with the solution.

The Benefits

Having multiple sets of eyes review a pull request before it gets merged to master or an integration branch, is a great way to catch defects early. At this time, they are usually still cheap to fix.

There are however much more important benefits. Instead of individual developers, the team is responsible for the internal and external quality of the code. This is a great remedy against the blame culture that is still present in many organisations. Managers or team members can no longer point fingers to an individual for not delivering a feature within the expected quality range. You tend to become happier and more productive, knowing that the team has your back. You can afford to make a mistake; someone will find it quickly.

Another effect is something called ‘swarming’ in Kanban. Because you are encouraged to help out on other branches before starting your own, you start to help others finishing work in progress. Stories are finished faster, and there’s a better flow throughout the system. Especially when stories are difficult, or when stories block other stories, it’s liberating to have people come and help you to get it done.

And of course, there’s all the benefits from the clear sense of code co-ownership. It’s invaluable to have a team where everybody knows what the code does, why it’s designed that way, how everything fits together. It also reduces the Bus Factor: no single team member is a bottleneck. Best practices are shared, and the code is more consistent. Opportunities for reuse are spotted before lots of duplication happens.

In short, pre-merge code reviews grow the team’s maturity.

For more information (tips and anti-pattern to avoid) read the original post that inspired this issue.

--> What do you think?