Wikimedia Release Engineering Team/Book club/How to do a code review

December 2019

Lars' summary[edit]

Do code reviews quickly and soon. Speed is more important than thoroughness.
- Keeps things moving forward, fewer snags, blockages.
Concentrate on things that can't be checked automatically. This includes automated tests and checks.
Change should make things better overall. It doesn't have to make things perfect.
Be courteous and considerate.
Reviewer doesn't need to provide improvements.
Explanations to reviewer questions should be in code or commit messages, not only to reviewer, for posterity.
Keep changes small and focused.

Lars: Surprisingly useful, but full of google-specific terms
Tyler: "approved" means a number of things for us
+1 means different things to different people
- Mukunda: I've heard that +1 means less than zero

Tyler: code review is (in general // for us?) a harsh pedagogical process
James: document reflects Google's culture: being right more important than being nice
Dan: complement sandwich is problematic -- antagonistic vs collaborative -- you both have the same motivations, rather than being a gatekeeper
Brennen: I liked the push that if you think an idea is bad, say something immediately: this is something we're (not just WMF) bad at
James_F: Yeah, avoiding the passive-agressive tack of having endless little issues -- if you ever see me saying "put a phab ticket together for this" == don't want to tell you this is a bad idea in code review, but in a wider forum
Lars: multiple reviews: (1) is this a good idea -- we need a different kind of review for that. I like saying this as early as possible rather than moving goal posts

Mukunda: best bit of the whole document: does this CL improve code-health? just merge it.
tyler: But doesn't define "better" or "code health"
Elena: Yeah, how do you know?
James_F: I think they are right to write it at this high-level. Documentation vs. design vs. testing vs. architecture is all in-scope but how do you pre-specify what is "better"…
James: Yes, this is good.

Brennen: Good in general. Yay comments.
Tyler: Checklist, could use side-by-side as doing code review, but maybe too prescriptive?
Brennen: "If you can't understand the code, other developers won't either".
Mukunda: Compare with https://www.mediawiki.org/wiki/Gerrit/Code_review#Review_checklist for us.
- Tyler: "Point out shortcuts and ask the developer to do a proper job." is a bit of a shortcut?
Dan: And our coding conventions.

Jeena: Makes sense.
Lars: "Changes shouldn't be large", but if it's small you don't need to navigate it?
James: +1, feels like this means Google's patches are way too large at times, in conflict with the size.
Tyler: No comment about the description being clear.

"One business day is the maximum time it should take to respond to a code review request (i.e. first thing the next morning)."

liw: the one that fails most frequently, not just for us but for everyone
brennen: fails particularly badly at the foundation. Google's emphasis might be the other extreme. When working async, I don't know if I want that expectation that code review happens in the same working day
liw: we are spread more globaly than Google, but a code review in a week would be nice
james: expecting comprehensive code review in a day feels like there is a serious amount of shared understanding about the system at a high-level and that you are submitting a small patch for a shared high-level understanding. This is not the case at the foundation, but it may be at google. Performance, architecture review takes longer when you are trying to understand the big thing they are trying to build. Maybe we're doing something wrong at the phab-level. I see patches that will not get code review in a week
Jeena: saying "I need a week" is something
james: we're so keen on not cookie licking that folks rarely say anything like "I'll review this soon".
liw: speed is more important than being correct, except you should not allow global-extinction level mistakes
Tyler: Distinction between time-to-first-response and time-to-LGTM; this is something we're thinking about here.
Tyler: Liked the "good enough for now"/timezones approach, don't get held up on the minor nits.
Mukunda: large change lists -- unblock the developer

James: I liked that the first bullet is "Be Kind", but I don't know why that is: could be for many reasons. It's not our first bullet point, but maybe it should be
Liw: my experience is that "Be Kind" is something you have to remind people about
James: the engineer you're talking to is part of the code review system. If you make them exasperated you're not making it better
Brennen: there's a really good essay about being an evangelical haskell developer and I think it speaks to a lot of this

James: there's a bit of stand on your honor stuff, but it was fine
Brennen: the bit about promised cleanup is good. People don't clean things up later.