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 Klimchuk