Hello People,
Few weeks ago at the flashrom meeting we had a discussion about code
reviews and the situations that sometimes happen. We decided that it
would be good to have some guidelines… and then we decided that I will
start writing something. I have been thinking about it since then. I
even started writing something, but I wasn’t happy at all: it felt
like composing a wall of text that no one would be reading.
I decided to think about: what problems are we solving? And then it
got much better. I gathered the list of problems that I think are
present and repeatedly happen during code reviews. All of the below is
RFC and of course, if there is a problem that is missing, please tell.
I would really appreciate everyone’s opinions on this!
What I suggest is: after agreeing on the list of problems, compose and
agree the answers to them (yes easier to say then to do :)), and then
it can be like “Code Reviews FAQ” or something like that? Especially
if we can encourage people to read the other point of view first.
—--------------------------------------
## Contributor point of view ##
—--------------------------------------
#1 Is this comment a conversation or an action item?
#2 Is this comment blocking patch from merging or non-blocking?
#3 Are we waiting for anyone [else] to have a look at this patch?
#4 Is my patch ready to be merged? / Why is my patch not being merged?
#5 Who are the best reviewers for this patch?
#6 This patch is a real emergency, what is the process for that?
—-----------------------------------------------------
## Reviewer / Maintainer point of view ##
—-----------------------------------------------------
#1 My important comment(s) got ignored and the patch was merged regardless.
#2 I have so many code reviews, and everyone wants it fast and
complains reviews are too slow.
#3 A patch that was merged broke something, but people don't want to revert.
#4 I asked the patch owner to fix something, but they are arguing with
me instead, and wasting everyone's time.
#5 What this patch is doing, and why is it needed?
#6 I want to review the patch, but I am busy for the next X weeks.
--
Anastasia.