Wikimedia Release Engineering Team/Book club/How to do a code review
Jump to navigation Jump to search
Notes on topic from Tyler: <https://gist.github.com/thcipriani/8dbce9b755c1d74617de34a153c2ecae>
- 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.
What to look for
- 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.