@mattab opened this Issue on January 5th 2015 Owner

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?

@mnapoli commented on January 5th 2015 Member

:+1:

@mattab commented on January 5th 2015 Owner

interesting read: The Pitfalls of Code Review (And How To Fix Them)

  • The impersonal nature of code reviews leads to tension and problems.
  • Code reviews devolve into nitpicking sessions.
  • Requests for review are being missed and take days/weeks/months to be considered.
  • Code reviews are highly subjective based on who is doing the review.

Code reviews are great for improving quality and keeping silly errors out of your code. And yet, they require care to ensure that they continue to be productive and effective tools. Like any tool, they can become more of a drain than an aid, and it’s up to us to ensure that we fix the problems and keep their usefulness.

@sgiehl commented on January 13th 2015 Member

I like the idea of code reviews.
Maybe we could already introduce new labels for PRs like 'needs review' and 'review in progress' or something like that. That would make it easy to see if there are review tasks pending/in progress.

@mnapoli commented on January 14th 2015 Member

+1 for the label, that's a really good idea to prevent PRs to be forgotten. It would also be easy to bookmark a github search to find all PR to review in all projects (including piwik pro).

@mattab commented on January 14th 2015 Owner

+1 for labels, I was wondering how we could manage this, and that's great idea!

@mnapoli commented on February 12th 2015 Member

Created a Needs Review label, feel free to rename if you have a better idea. Let's start using it!

@mattab commented on February 13th 2015 Owner

FYI @piwik/core-team From now on, you can please add a label Needs review whenever you create a pull request. Someone from the team will then review it. See this issue for more details (we hope to improve quality and code ownership amongst the team!)

@mnapoli commented on February 15th 2015 Member

@mattab is there any reason to keep this issue open now? The RFC can be closed I guess, it's been open for more than 1 month and nobody seemed against that.

@mattab commented on February 15th 2015 Owner

sounds good, created little follow up issue: Add info about Pre-merge Code Reviews in the "contributing to Piwik" guide #7210

@mattab commented on March 9th 2015 Owner

I have a question for core team developers: after we have reviewed a pull request, would it make sense that the reviewer remove the Needs review label, and maybe re-assign issue to the original developer?

@tsteur commented on March 9th 2015 Owner

or remove the label and merge directly?

@mattab commented on March 9th 2015 Owner

I was wondering, only in the case when the PR has actually some feedback that needs to be acted on, code to be changed, etc.

@mnapoli commented on March 9th 2015 Member

A common practice I see in open source projects is to have one-two people do the review and comment the PR. Either there are things to discuss or fix, either it's all good (usually people write LGTM or :+1:). Then the person that opened the PR can merge.

@mattab commented on March 12th 2015 Owner

Note: one of the important things to check for in pre-merge core reviews is that all new functionality is covered by tests, ideally unit tests. We need to make sure that we don't introduce as few bugs as possible and this mix of Pre-merge code reviews + Ensuring tests cover it all, should make huge positive difference :+1:

This Issue was closed on February 15th 2015
Powered by GitHub Issue Mirror