Nguyen Le Phong

友善而高效的工程文化第 3 篇,共 5 篇

如何写一个让人真正乐于审查的 Pull Request

一个出色的 pull request 是给审查者的礼物:简小、描述清晰、容易说是。可审查 PR 的解剖学——大小、标题、描述、提交卫生和自审——附具体的前后对比示例。

你肯定遇到过这种情况:周二下午,你的审查队列里出现了一个 pull request,标题是"fixes",diff 跨越 41 个文件共 2347 行,没有描述,评论里只有一个轻快的"PTAL"。你的心一沉。还是打开了。二十分钟后你把同一个方法看了三遍,仍然不知道它解决了什么问题,只留下了一条忐忑的评论:"总体上看起来不错,也许把这个变量改个名?"你刚刚给一段你没看懂的代码盖了橡皮图章。

作者并非恶意——他们可能全神贯注于工作,为自己的成果感到骄傲,只是忘记了交付代码只是工作的一半。另一半是让它被审查、被理解、安全地合并。Pull request 不是合并前的手续;它是一次对话,像所有对话一样,当有人付出努力让对方理解时,效果会更好。

本文就是关于这种努力的:如何写出队友真正乐于审查的 PR——简小、有目的、易于跟进,也容易说"是"。

PR 是沟通,不只是代码

在第一行 diff 之前,pull request 首先是给另一个人的一条信息。那个人时间有限,对你晚上十一点在想什么缺乏上下文,还有十几件其他事情在争夺他的注意力。你作为作者的工作,是让他们的工作变得简单。

把审查者当作你的受众,而不是你的法官。他们不是来抓你把柄的——他们是帮你交付稳健成果的合作者。你越帮助他们理解变更背后的为什么,审查就会越快越好。一个出色的 PR 描述就像是你代码的求职信:它铺垫了场景,解释了动机,并告诉审查者具体看什么。

这种思维方式的转变改变了一切。一旦你停止为自己写 PR,开始为审查者写,你自然会希望把它写得更小、描述得更好,并在请人看之前整理好。

审查者的黄金法则

如果审查者必须通过读代码才能理解为什么这个变更存在,PR 描述就没有尽到职责。代码解释了什么发生了变化;描述解释了为什么它需要变化

保持简小而聚焦

改善 pull request 最有效的一件事就是把它写小。PR 越大,审查就越不彻底,合并时间越长,漏过的 bug 也越多。

一个好的 PR 包含一个逻辑变更。不是一张工单,不是一个功能,不是一个 sprint 的工作——而是一个连贯的想法,可以被独立理解、测试和回滚。"添加用户头像上传"是一个变更。"添加用户头像上传并重构 profile controller 并升级三个依赖"是三个变更穿了一件外衣。

As PR size increases, review quality drops and time-to-merge rises. PR 大小(变更行数) 对审查的影响 审查质量 (由高到低) 合并时间 (由低到高) 小型 PR < 400 行 大型 PR > 1000 行
随着 PR 体量增大,审查者从仔细阅读变成快速扫描,从认真思考变成橡皮图章,bug 悄然溜过。小型 PR 获得更好的审查,并且合并更快——两个结果同时改善。

如何拆分大型工作

大型功能不必以大型 PR 的形式出现。几个实用的拆分策略:

  • 将重构与行为变更分开。第一个 PR:重命名、重组、提取——没有新逻辑。第二个 PR:实际的功能。审查者可以验证重构没有副作用,然后完全专注于新逻辑。
  • 先提交基础设施。添加数据库列、迁移、类型——全部不带 UI。然后添加使用它的 UI。每个 PR 可以独立部署。
  • 使用功能开关。把未完成的工作合并到默认关闭的开关后面。PR 小而安全;功能在翻转开关之前还不算"完成"。
  • 堆叠 PR。PR B 基于 PR A。一些团队使用堆叠 PR 工具独立审查每一层。如果你的团队没有堆叠工具,保持简单:先完成并合并 A,然后开 B。
一个实用的启发

如果你无法用一句话、不带"并且"两字来描述你的 PR,它可能包含了多于一个逻辑变更。找到接缝,拆开它。

写一个值得一读的标题和描述

标题是标题行。它应该用一行说明什么发生了变化以及为什么重要。避免像"update"、"fix stuff"或"changes"这样模糊的动词,追求具体性。以下是常见的 PR 标题坏味道及改进方法:

PR 坏味道问题所在更好的版本
"fixes" 完全没有上下文——审查者必须读每一行来猜发生了什么。 "修复并发会话碰撞时购物车结账的竞态条件"
"WIP: various changes" 表明作者还没准备好。草稿 PR 的存在就是为了这种情况。 在真正准备好审查之前作为 GitHub 草稿开着。
"Update UserService" 描述了在哪里,而不是什么或为什么。每个 PR 都在更新某些东西。 "为 UserService 添加邮件更改验证流程"
"JIRA-1234" 迫使审查者打开第二个标签页才能理解动机。 "将每 IP 的 API 速率限制为 100 次/分钟(JIRA-1234)"——工单作为上下文,而不是替代品。
"Refactor + new feature + bump deps" 三个 PR 穿了一件外衣。每个关注点都应该有自己的审查。 拆分为三个独立的、有焦点的 PR。

描述是真正沟通发生的地方。一个好的描述回答了审查者在最初十秒内提出的三个问题:

  • 什么发生了变化,在哪里?
  • 为什么需要这个变更——背景、bug、用户故事?
  • 如何验证它有效——测试步骤、截图、受影响的边界情况?

来看一个前后对比,让它更具体:

--- 改之前(2000 行的 "fixes" PR)---

标题: fixes

描述:(空)


--- 改之后(为审查者写的同一变更)---

标题: 修复并发会话时购物车结账的竞态条件

## 是什么
同一用户的两个并发结账请求都可能在任何一方递减库存数量之前通过库存检查,
导致超卖。本 PR 在库存递减操作周围添加了行级数据库锁。

## 为什么
客服在过去 30 天内反映了 12 起超卖事件(见 Sentry issue #4421)。
根本原因是两个请求在 inventory 表的 SELECT 和 UPDATE 之间产生竞争。

## 如何测试
1. 运行新的并发回归测试:npm test -- cart.concurrency
2. 在 staging:打开两个浏览器标签,对库存为 1 的商品同时点击"购买"。
   只有一个应该成功。

## 风险/备注
- 锁定范围仅限于单个商品行;不同商品同时结账不受影响。
- 无需迁移——使用现有表的 SELECT FOR UPDATE。

提交卫生

如何组织你的提交很重要——不只是为了满足流程,而是因为提交是变更如何形成的永久记录。一年后,某人(可能是你)会在一行代码上运行 git blame,然后沿着提交哈希追溯历史。他们找到的应该讲述一个故事,而不是一段意识流。

一个好的提交信息遵循约定格式:一个简短的祈使句主题行(50 个字符或更少),一个可选的空行,以及一个可选的正文解释为什么——而不是 diff 已经显示的内容。

# 约定格式:
type(scope): 简短的祈使句主题

可选正文:这次变更的原因,它解决了什么问题,
任何不明显的决策及其背后的推理。

# 真实示例:
fix(cart): 添加行级锁以防止并发结账时超卖

feat(auth): 添加邮件更改验证流程

refactor(profile): 提取头像上传为独立服务

除了信息格式,还要考虑提交的形态。每个提交应该代表一个连贯的步骤——重构、测试、新行为。查看单个提交(而不是整个 diff)的审查者,应该能跟上逻辑进展:"提取方法 → 添加测试 → 实现行为"比二十个在最后一刻压缩的"WIP"提交讲述的故事更好。

在开 PR 之前,使用交互式 rebase 整理混乱的分支。你不需要从一开始就有完美的提交——在请人看之前再打磨。

请人审查前先自审

在 ping 队友之前,先成为自己的第一个审查者。在 GitHub(或你选用的代码托管平台)上打开 diff,当作从未见过这段代码来读。这种上下文切换抓到的问题比你预想的更多:遗留的调试输出、增长得太大的函数、忘记移除的 import、曾经为真但现在不对的注释。

值得过一遍的自审清单:

  • 从头到尾读 diff。不要快速浏览。假装自己第一次看到它。
  • 检查调试遗留物。console.log、标注"待删除"的 TODO 注释、注释掉的代码块、临时硬编码的值。
  • 验证测试是否存在。如果你添加了逻辑,有测试吗?如果你修复了 bug,有回归测试吗?
  • 寻找无意改动。空白字符变动、被重新格式化的文件、意外的范围扩大。把它们移到单独的 PR 或者还原。
  • 自己留下引导性评论。如果某个部分不明显,在审查者看到之前先加一条 PR 评论解释推理。这不是软弱的表现——这是体贴。
一个简单的规则

如果你不愿意让一个高级工程师现在立刻、不加解释地审查这个 PR——它还没准备好。先修改,再请求审查。

自审也有自私的好处:它在问题变成永久的审查历史评论之前抓住了令人尴尬的错误。每个人都有一个故事:按下"请求审查"后十秒就发现了那个拼写错误。十分钟的自审消除了大多数这类情况。

回复审查反馈

PR 不是在你推送代码时就结束了——而是在它合并时才结束。在这两个事件之间是审查循环,你如何处理反馈与代码本身同样重要。

保持审查推进、关系不受损的几个原则:

  • 回复每一条评论。哪怕只是说"已完成"或"好发现,在最新提交中已修复"。沉默让审查者不确定你是否看到了他们的备注。
  • 推送修复时描述你做了什么。"在最新提交中修复了——把逻辑移到了辅助函数里并添加了测试"比一个没有上下文的新提交要清晰得多。
  • 审慎地解决线程。在大多数团队,评论者满意后才解决线程。不要在没有确认反馈的情况下自己关闭。如果你不确定约定,就问。
  • 有礼貌地不同意。如果你认为某个建议是错的,清楚地说出来并给出理由——不要就是忽略它。"我考虑过那个方案,但[原因]——如果你有强烈感觉,很乐意讨论"让对话保持专业和建设性。同时也可以参见关于如何善意地审查代码——审查者对同一对话的视角。
  • 明确地重新请求审查。解决反馈后,不要等审查者自己注意到。重新请求审查,并留下简短摘要:"解决了所有评论。主要变更:提取了工具函数,添加了两个测试,因为 Y 原因保留了 X 的原始方案。"

PR 规范如何随团队规模变化

好 PR 的标准不是固定的——随着团队成长和代码库成熟而变化。以下是期望如何演进的:

团队背景最重要的是什么实用规范
独立 / 自由职业 未来的你是审查者。PR 是六个月后你回看代码时决策的文档。 即使没有审查者:一个清晰的标题,描述中简短的"为什么"。让 git log 以后能读懂的提交信息。
小型团队(2–8 人) 速度很重要。大家都了解代码库。审查是对话式的。信任度高。 保持 PR 小而快。描述可以更轻——一两句上下文。当天回转的异步审查是合理的期望。
中型团队(10–50 人) 审查者可能不了解完整上下文。新人持续加入。变更跨越团队。 完整的是什么/为什么/如何描述。UI 工作的截图。测试计划。跨团队变更需要额外关注,可能需要更广泛的审查。
大型 / 企业(50+ 人) 审查者有时是陌生人。合规、审计追踪和正确性审查很重要。有回转 SLA。 工具强制执行的正式 PR 模板。必要的批准。敏感变更的安全审查。设计文档和工单的链接。高风险变更的回滚计划。

四人创业团队不需要每个 PR 描述里都有回滚计划。银行需要。把仪式感与风险匹配,随着团队成长重新审视规范——十个人时有效的做法在五十人时会感觉不够用。

核心要点

  • PR 是给人的信息。为你的审查者而写,而不是为自己。他们的时间和上下文都是有限的。
  • 保持简小而聚焦。每个 PR 一个逻辑变更。将重构与行为变更分开。使用功能开关把未完成的工作安全地合并到开关后面。
  • 标题 + 描述 = 定位。回答什么发生了变化、为什么需要变化、如何验证。代码展示了什么;描述提供了为什么。
  • 整洁的提交历史讲述一个故事。约定式信息,逻辑提交,请求审查之前整齐的 rebase。
  • 先自审。在其他人看之前,在 GitHub 上读你自己的 diff。留下引导性评论。自己抓住明显的错误。
  • 关闭反馈循环。回复每一条评论,解决备注后重新请求审查,有分歧时给出理由——而不是沉默。
  • 把仪式感与团队规模匹配。四人创业团队和 500 人银行需要不同的 PR 规范。随着成长重新审视你们的规范。

下一步是桌子的另一边:接收你 PR 的人应该如何给出有帮助而不是伤害的反馈。如果你曾经精心写了一个 PR,却收到了简短的"这是错的,重写吧",你会欣赏有效的反馈——这篇文章的续集。

常见问题

pull request 应该有多大?
经验法则是,目标在 400 行变更以内。更重要的是从逻辑范围而不是原始行数来思考:一个 PR 应该包含一个可以被独立理解、测试和回滚的连贯变更。如果你无法用一句话、不带“并且”两字来描述 PR,它可能太大了。
什么样的 pull request 描述是好的?
一个好的描述回答了审查者在最初十秒内提出的三个问题:什么发生了变化,在哪里?为什么需要这个变更?如何验证它有效?具体来说:一个简短的“是什么”部分总结变更,一个“为什么”部分给出动机或 bug 背景,以及一个“如何测试”部分含复现步骤或测试命令。对于 UI 变更,一张截图胜过千言万语。对于敏感变更,风险和回滚说明是加分项。
如何拆分一个大型 pull request?
三个策略配合使用效果很好。第一,将重构与行为变更分开:在一个 PR 中重组(没有新逻辑),然后在下一个 PR 中添加功能。第二,先提交基础设施——迁移、类型和脚手架——再提交使用它的 UI,这样每个 PR 可以独立部署。第三,使用功能开关把未完成的工作安全地合并到默认关闭的开关后面。如果你的团队有工具支持,堆叠 PR(PR B 基于 PR A)是更高级的选项。
我应该先自审 pull request 吗?
是的,始终如此。在请求审查之前,在代码托管平台上打开 diff,当作第一次看来读。寻找遗留的调试输出、缺失的测试、无意的空白变动以及不明显的逻辑。在审查者到来之前,对任何需要上下文的地方留下引导性评论。自审在错误变成永久的审查评论之前就抓住了它们,也是对审查者时间的尊重。
提交信息应该包含什么?
提交信息应该有一个简短的祈使句主题行(50 个字符或更少),描述提交做了什么。使用约定式提交格式:type(scope): subject——例如 'fix(cart): 添加行级锁以防止超卖'。如果原因从 diff 中看不明显,在主题后面加一个空行和简短的正文,解释动机或任何不明显的设计决策。diff 展示了什么发生了变化;信息应该解释为什么。