3 ms·
> Nobody reviews it It does take some culture shifts to make reviews a normal part of the culture. Send reviews to specific people and expect deadlines. (Somet
by WorldMaker 4y ago
> Nobody reviews it
It does take some culture shifts to make reviews a normal part of the culture. Send reviews to specific people and expect deadlines. (Sometimes this is assigning "code owners" for specific areas. Sometimes this is "have a review buddy".) Add bots and/or calendar reminders if you need to poke someone to get the review done. If "nobody" is the right person to review it, don't have a review at all. It's fine to say that changes to certain areas or below a certain size don't need another reviewer if they pass builds/tests. I've seen teams with bots that auto-merge small PRs after a specified amount of time with no review comments. I've heard of teams that set "these types of PRs are fine to self-merge if" checklists for applications with active QA teams where it's fine to merge code quickly for quick feedback and iterate in further PRs.
> You end up in an endless back and forth with one or more reviewers (e.g. push-review-fix-push-review-fix-push-review...). If this occurs over the course of days, the "fix" portion becomes an interruption where you have to remember the context of the changes, make the changes, then submit them.
First, automate anything that can be automated. Use linters. I prefer the sorts of opinionated linters/formatters at this point that auto-format rather than just complain (prettier for the web stack is a strong example). Once automated remove "style" problems as something to ever mention in a PR once the automated checks have passed.
As a review submitter, find ways to ask where the balances between "cosmetic", "suggestion", "tech debt", and "show stopper" are. Eliminate cosmetic comments (again, if it matters configure it in a linter and automate it away). Work on the confidence to ignore suggestions/tech debt (learn from them, but don't feel compelled to answer every single one with a response or a commit). One key way to to both "ignore suggestions/tech debt" and find that balance between what is actually a "show stopper" is variations on the very simple question "Can we move this to the backlog for a follow up PR?" or "Can I keep this in mind for the next PR for this feature?" A lot of things that sound like show stoppers are merely suggestions, but in the context of a PR it can sometimes feel like everything is important that the reviewer mentions.
As a reviewer, find better ways to set those expectations up front. Mark suggestions as suggestions. Sometimes just adding "Suggestion:" in front does so much to set expectations on how important a note/comment is. Use more phrases like "if you have time" or "something to keep in mind for next time". It's too easy to just note every single thing "wrong" you find and forget to mention that most of what you found "wrong" doesn't matter in short term. I find it is also useful to be proactive sometimes: if it is a tech debt solution that can wait, merge the PR and add the tech debt item to the backlog mentioning the PR without "making a full round" of it.
Some of that is setting expectations for when a PR is "done". It's okay to expect PRs to be "done" quickly and to instead expect follow up PRs. Putting too much pressure on "a PR needs to be perfect" gets in the way of the ideals of continuous integration. You want code interacting sooner rather than later. You can fix up the suggestions and tech debt next time, but it's sometimes important to have that extra time for integration problems to be discovered and QA testing to happen on the "not quite perfect" PRs.