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.
piwik/piwikrepository as well as all
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?
interesting read: The Pitfalls of Code Review (And How To Fix Them)
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.
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.
+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).
+1 for labels, I was wondering how we could manage this, and that's great idea!
Created a Needs Review label, feel free to rename if you have a better idea. Let's start using it!
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!)
@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.
sounds good, created little follow up issue: Add info about Pre-merge Code Reviews in the "contributing to Piwik" guide #7210
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?
or remove the label and merge directly?
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.
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.
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: