@mattab
The goal of this issue is to note in the guide "Contributing to piwik core" at http://developer.piwik.org/guides/contributing-to-piwik-core that we use Pre-merge code reviews, that issues can be tagged with Needs review tag. Maybe we could note in this guide the Rules / Benefits / Pitfalls (see #6916).
Interesting article How to write the perfect pull request? that we could link to, or reuse content from, in our contributing guide
also:
- Take enough time for a proper, slow review, 30-60 min.
- Authors should annotate source code before the review begins.
- Establish quantifiable goals for code review and capture metrics so you can improve your processes.
- Checklists substantially improve results for both authors and reviewers.
- Verify that defects are actually fixed!
- Authors must foster a good code review culture in which finding defects is viewed positively.
- Beware the “Big Brother” effect.
- The Ego Effect: Do at least some code review, even if you don’t have time to review it all
- Lightweight-style code reviews are efficient, practical, and effective at finding bugs.
Very interesting: The Pitfalls of Code Review (And How To Fix Them)
In particular this:
I use a three-tier system for code reviews. Comments that begin with “You might…” indicate that I am making a suggestion, based on my experience. The developer can choose to ignore my suggestion or not, based on their experience. “You should…” comments indicate something I feel strongly about. The developer is strongly encouraged to consider the suggestion, but again, I leave this to their discretion.
Comments beginning with “You must…” are not optional. But these kinds of comments are reserved for security vulnerabilities, coding standard violations, obvious bugs, or other serious code problems. These are the only blocking code review comments, meaning that all other code review comments can be ignored, but these must be dealt with to obtain my approval for the patch. Everything else gets a thumbs-up.
--> What do you think of adopting this system for our pre-merge code reviews?