At Trainline (where I currently work as an iOS developer) on the iOS team, a pull request (PR) needs at least one approval before it can be merged. That approval can be from anyone on the team. I think this policy works well for our situation ("our situation" being the size of our team and the size of our codebase).
When I worked at Google, they had a much more formalized process. The process is a bit tricky to explain so bear with me. First, I need to explain 2 concepts:
code owners
readability
Each project at Google has an OWNERS file with a list of names in it. Generally, you are an owner of that project if you
currently work on that project
or if you have contributed to it a significant amount in the past.
Readability is related to programming languages and code style. A developer at Google can "get readability" in one or more languages. For example, I was in the process of getting Objective-C readability when I was there. A lot of my co-workers had Java readability (because they worked on Android stuff). You generally want to get readability in the language(s) you work with. When I was there, there were 2 ways to get readability (I don't know if this has changed since I left).
With a single PR: You could submit a PR for review by a "readability reviewer" (this is just someone who has volunteered to help out with the readability program - I don't know if they receive any sort of special training). I think there were some requirements for the size and content of this PR, but I don't remember exactly. The readability reviewer looks over the PR to make sure it meets certain criteria and follows the Google style guide for the given language. And then they either grant you readability or they provide feedback and you can adjust the PR until the readability reviewer has no more feedback and then they grant you readability.
Incremental readability: You add a readability reviewer to most of your PRs so that they can assess and give feedback. And once they have seen enough of your work to demonstrate that you are fluent in the Google style guide for your language + other best practices in your language, you are granted readability.
This is a bit of a tangent, but I must mention - the waitlist for getting into the readability program was generally VERY long when I was there. Like upwards of 6 months. For some languages, the readability waitlist is more than 1 year. So actually getting readability in your language of choice was quite a bottleneck. Anyway - back to code review.
A PR at Google needed approval by
someone who was an owner of the code
someone with readability in the language the PR is written in
someone who is not you
in order to be merged.
Some example scenarios:
I create a PR for the project I work on (ie I am a code owner) but I don't have Objective-C readability. In this case, I need someone who does have Objective-C readability to approve my code.
I create a PR for the project I work on and I have Objective-C readability. In this case, I just need anyone who isn't me to approve my code.
I want to contribute to a project that I don't work on (so I'm not a code owner for the project), and I don't have readability in the language. In this case, I need someone who is an owner and someone with readability to approve the PR (can be the same person).
So, as you can see, Google's process is very formalized. But of course, they are a huge company with tens of thousands of developers all working on one huge codebase. I thought that system worked pretty well, other than the wildly long waitlist for readability (could be easily solved if there were more readability reviewers - but that was volunteer-based, so there weren't).
The two other places I worked before Google both had similar code review processes to my current job - essentially: anyone can review anyone's code and only one approval is required for a PR to get merged.
I have never worked anywhere that required more than 1 approval for a PR to be merged. I'd be curious to hear about the ins and outs of this process if anyone has experienced that or something similar. I have also never worked anywhere with any sort of explicit requirement about the seniority of the reviewer. And I don't think seniority requirements are necessary (but I'm open to other opinions!). I see no problem with the newest developer reviewing the most veteran developer's code. I see no problem with the most junior dev reviewing the most senior dev's code. Even the newest team members can catch mistakes in the code. Plus, I think code review is a great learning opportunity (I have learned a TON from looking at the way my co-workers write code). And if there is something in the PR that the reviewer does not understand, they can simply ask for the necessary information or context. They can also always ask for someone else to step in and help them review the code or to give it an additional review.
When I am new to a team, I often don't feel fully equipped to review a PR. However, I have been re-thinking this lately. I am thinking more and more that new team members should jump right into the code review "rotation" (whatever that team's process is) because it's such a good learning opportunity. (As long as time is not too much of an issue, because it will certainly take them longer to review the code than someone with more context.) I think they should simply feel free to litter the PR with questions - or try to review it on their own and write down all their questions (to avoid cluttering the PR) and then chat with the PR author or another team member to get their questions answered. Of course, I think the team should discuss this approach together first so everyone has accurate expectations of the code review process. As a new joiner, I wouldn't just go ahead and throw myself into the code review process without first discussing it with my team and/or manager.
Those are all of my thoughts on who can review code! Thanks for reading. As always, I'd love to hear what you think! I'm always interested in different approaches and perspectives.
コメント