A lot of developers are confused about who should mark PR comments as resolved. There are many code review tools but there is no defined process for resolving PR comments.
Hence, sometimes the PR reviewers resolve their respective comment(s) and approve the code to be merged into the master branch. Sometimes the PR author does the work of marking comments as resolved, and the PR enters the CI/CD pipeline. There is no standard as such, and it might appear that there is no right way of resolving PR comments.
In this blog, we discuss the same, who should mark PR comments as resolved? The PR author? Or the PR reviewer who added a comment?
What Are PR Comments, PR Author, PR Reviewer, and PR Maintainer?
PR Author
The developer who raises a specific PR request is a PR author for the same.
PR Comments
After PR is raised, domain expert(s) get assigned to the pull request to review it. These reviewers may add a comment as feedback, a suggestion, a question, or a request to make a change in code to improve the functionality, and readability, or to comply with the defined code quality conventions (if any). These comments are referred to as PR comments.
PR Reviewer
The reviewer is called the PR comment author, aka PR reviewer or assignee. The PR reviewers only guide the PR author to implement the suggestions for improving the code quality. PR reviewers don’t solve the issue for the PR author. Rightly so, PR reviewers could be internal team members or they could even be external reviewers. Hence, their role is only to share their expertise and guide the PR author.
PR Maintainer
A maintainer is someone who approves the PR to be merged into the master branch. Marking PRs as ready to be merged is a right that always remains with the maintainer.
The Current PR Review Process
Once the PR reviewer adds feedback, the status of the conversation changes to ‘Active’.
Now, the PR author has two options:
1. Pushback on PR Comments and Request Resolving it Without Any Rework
In some cases, the PR author can rightfully deny abiding by the suggestions of the PR reviewer.
This is because-
- How a feature or code logic is implemented can be subjective.
- At times, the PR reviewer nitpicks trivial changes that are not important and can be skipped for better software delivery efficiency.
- In open source projects, a lot of comments are from contributors which are often trivial.
So, in such circumstances, what if the PR reviewer & the PR author are at the tug of war? Who will resolve the PR comments now?
2. Implement the changes
If the PR reviewer’s comment is critical to the software quality, the developer reworks the code and commits it back for the PR reviewer to review it again.
Again, the problem is the absence of a clear procedure to mark the PR comment as completed.
There is confusion about whether the PR reviewer should always resolve the comments or sometimes the PR author can mark comments resolved too after the code rework.
Why is PR Comments Approval a Problem?
One can advocate that the PR author has made the necessary changes and updated the code. So, nothing wrong with the PR author marking PR reviewer comments as resolved. This could also speed up the code review process by minimizing the back-and-forth communication between the PR author and the PR reviewer.
But there is a caveat.
A PR author might be biased in assessing the efficacy of the solution.
If they self-approve the issue raised by the PR reviewer, it defeats the purpose of code review. There is a high chance that if the PR authors themselves mark the comments as resolved, the code vulnerabilities may make their way into CI/CD pipelines, and possibly into the production environment as well.
That’s a huge risk.
But if PR authors aren’t allowed to resolve on their own, and if they have to wait for prolonged times for the reviewer to be available, wouldn’t this be a big impediment to developer productivity?
Not to mention, it would also hamper the maker time of the developer, as this would need frequent context switching between active code development and PR comments implementation.
In fact, in the absence of a proper PR review tools stack, a developer may spend productive hours shuffling between the PR comments looking for which ones are addressed and which ones are still open.
These are all valid propositions. But giving the reigns of resolving PR comments in the hands of PR authors is just too risky, and thus not recommended.
Who Should Approve the PR Comments?
Well, as a convention,
- It’s no harm to always let the authority lie with the PR reviewer.
- PR reviewers are folks who have years of experience & thorough understanding of the code base.
- PR reviewers have a more polished approach to coding, and could better make decisions around design patterns for application scalability and maintainability.
- By allowing PR authors to self-approve the PR comments, the opportunity for them to learn the nuances of software architecture and coding best practices is lost or delayed.
- When satisfied with the solution, PR reviewers can mark their comments as resolved, aka approved.
- If they are not satisfied with the solution, they can mark PR review comments as unresolved and may drop a descriptive reason explaining the decision.
- This approach to PR comment approval not only helps improve the code quality & security, and software performance metrics but also saves teams from accruing unnecessary tech debt.
- It’s a good idea to involve & delegate the code review responsibility to the next domain expert in case of a rift between the PR author & PR reviewer.
- Under no circumstances, a code should be allowed to move to CI/CD pipelines without necessary checks.
Should the PR Author Approve the PR Comment?
Ideally, NO.
But there could be exceptions (though not recommended). When the changes are trivial, PR authors might be allowed to mark the comments as completed. When the code commits only involve fixing a typo, removing commented methods, lazy classes or refactoring code smells such that it does not change anything in terms of functionality, quality, and performance; the PR author may self-resolve and request the repository maintainer to trigger the CI/CD pipeline.
How to Make Your Code Review Process Flawless?
An engineering analytics tool that gives you clear visibility into your engineering work helps you to kill the pain out of your code review process. Hatica enables you to track key code review metrics to understand bottlenecks in code review cycle, and improve collaboration:
With Hatica's review collaboration dashboard, developers can easily collaborate on code reviews, track review progress, and identify areas for improvement.
