Several teams at my company practice a code review workflow I've never seen before. I am trying to understand the thinking behind it, with the idea that there's value in making the whole company consistent. (I contribute to multiple codebases and have been tripped up by the differences in the past.)
- Code author submits a pull request
- Reviewer examines the code
-
- If the reviewer approves, they leave a comment along the lines of "Looks good, feel free to merge"
- If the reviewer has concerns, they leave a comment like "Please fix minor issues X and Y, then merge" (For major changes, return to step 2)
- The code author makes changes if necessary, and then merges his or her own pull request
I have the following concerns:
-
In the case of approval at step 3, this workflow creates a seemingly-unnecessary roundtrip to the pull request author. The reviewer, who is already looking at the code, could just merge it immediately.
-
In the case of changes being requested at step 3, the agency to merge the pull request now rests solely with the PR's author. No one besides the author will look at the changes prior to merging.
What are some other advantages or disadvantages to this workflow? Is this workflow common on other engineering teams?
Best Answer
In the first case, it's usually a courtesy. In most organizations, merges kick off a series of automated tests which must be dealt with promptly if they fail. Especially if there was a significant delay between when a pull request was submitted and when it was reviewed, it's polite to allow it to be merged on the author's timetable, so they have time to deal with any unexpected fallout. The easiest way to do that is to let them merge it themselves.
Also, sometimes the author becomes aware of reasons later that a pull request shouldn't be merged yet. Maybe another developer's PR is higher priority and would cause conflicts. Maybe she thought of an uncovered use case. Maybe a review comment triggered a brainstorm that needs further investigation before the issue is fully satisfied. The author knows the most about the code, and it makes sense to give him or her the last word about when it gets merged.
On the second point, that's just a matter of trust. If you can't trust people to fix minor issues without being double-checked, they shouldn't be working for you. If the issue is big enough that it will need another review after the fix, then trust reviewers to ask for one.
That being said, I do occasionally merge other author's pull requests, but it's usually either very simple changes, or from external sources, where I personally take responsibility for shepherding through any test automation failures.