[译] Pull request review 的十大错误

• 原文地址：Top ten pull request review mistakes
• 原文做者：Scott Nonnenberg
• 译文出自：掘金翻译计划
• 译者： luoyaqifei
• 校对者：xiaoyusilen，phxnirvana

Pull request review 的十大错误

我已在多个 GitHub 托管的项目中工做过，不管是 我的的、 开源的 或者 工做中的。有的时候使用公开的 GitHub，别的时候使用 GitHub 企业版。可是有一件事情是同样的：提交一个 pull request 实在是简单，可是很好地 review 一个 pull request 实在是有点难。

为了不更多的麻烦，本文会涉及到十大 pull request review 错误和一些怎样作得更好的想法：

1. 漫不经心 +1

这是那么地让人难以抗拒：某个 pull request 实在太大，提交者又是你很信任的人。他们已经在这段代码上工做了好久，并且这段代码老是工做得很好。更没必要说，你也有你本身的 deadline 要赶啊！

+1
看起来挺好
走起，合并吧！复制代码

不要再这么作啦！

你真的须要花一些时间来 review 这段代码。每一个人都会犯错——资历水平并非什么对抗错误的神奇守卫。而且，你要清楚本身的身份，做为一个 reviewer，你的身份是使用你的创造性和专业性，使用任何方式来减小 pull request 将代码库变得更糟的状况。

这才是真正的目的，对不对？若是每一个 pull request 都让代码库变得更好，项目极可能是有长期潜力在的！

2. 拖延

为何如今就要 review 它呢？毕竟这真的是个大 pull request 啊。你当下的任务过重要了。你最终会绕着它走，对不对？或者，可能你只是等着别人插手吧……

拷问下你本身的心里吧！让那股力量从你的心中流过！在那种阻力下，你可能会有一些真正的顾虑。

既然你已经认清了你真正的顾虑，行动起来！

• 若是对于更改到底作了什么，代码提交者没有提供足够的指引，直接去问提交者要这些！好比说，原始需求在哪？
• 若是只是由于更改太庞大，难以一次 review 完毕，让他们分红屡次提交！
• 若是你不明白什么，放下你的骄傲，问！
• 若是你发现了足够多的问题／有足够多的顾虑，多是时候与提交者作一些面对面的互动了。

3. 统一 diff

你在 review 不知所云的代码吗？在 GitHub 和 GitHub 企业版上默认的 diff 显示是「统一的」（Unified）(译者注：如今 GitHub 已经默认使用 Split 显示了)。在这种模式下，渲染一系列文件的更改，软件看的是被添加的／被删除的行，而且尝试着将更改块智能分组，全部的都是内联的。可是你能看懂多少更改呢？在多数状况下，「统一的」 diff 很难阅读。所谓智能的块选择真的不够智能。

好消息是 GitHub 和 GitHub 企业版都支持将 diff 「分屏」（Split）。左侧是旧文件，右侧是新文件。若是代码被移除，你将在右侧看到空区域；若是代码被添加，你将在左侧看到空区域。这二者均可以让你清楚地看到文件在更改先后的模样，可以促使你作更好的 review 决策。

不要为莫名其妙买单。在 diff 的右上角点击「将 diff 分屏（Split）」吧！

4. 专一样式而不是实质

在 pull request 的 review 时，即使对代码风格和格式有异议，也不该在这些方面浪费时间。我以前写过一篇文章，关于 使用 ESLint 这类工具来将这些事情彻底自动化的必要性。为何？由于它彻底是浪费时间！

一个好的 reviewer 会将时间放在尝试着去理解 代码更改的最终目的 上，经过回溯到原始的需求。有一个追溯这段更改的工做项吗？有相应的测试用例吗？它到底在要什么？

只有掌握了这些代码背景，真正的 review 才会实现。可能在浮于表面的结构／样式 review 时看起来合理的代码，当理解了终极目标后会变得不能接受。

固然，你可能会回避惹上这种「大」事情，毕竟有如此多的时间被消耗在已有的更改上了。可是，讨论更好的解决方案是值得的。对于每一个人来讲，这都是一个学习的机会。你可能错误地相信会有一个更好的解决方案，可是这种相信会引领你和原始提交者进行一次讨论，来肯定到底有没有更好的解决方案。

5. 不注意未完成的更改

差别对于展现已有的更改很棒，可是这也是问题所在。从定义上就能够看出来，它并不能展现出什么 没有 被更改。时刻观察有哪些更改应该被更普遍地应用，好比说查找／替换这种可能没有覆盖到整个代码库的状况。

或者一个更改应该涉及到一整个组件而它只涉及到其中一部分的状况。

或者彻底缺乏测试的状况。测试是更改很重要的一部分，可是它其实是很容易被遗忘的，若是它们根本不在 diff 里面的话。你很难由于被提醒而想起它们。

不得不认可，这真的很难！这是 review 里最难的类型。它可能帮助你作一些快速的明智的检查搜索，或者在提交者的分支上，或者在你本身的机器上有的任何的东西。或者，你能够问提交者他们在你能看到的代码更改以外，作过哪些类型的全面检查，

6. 轻视测试代码

一旦在 pull request 里有一些测试代码的更新，就很容易被麻痹在一种错误的安全感里。若是他们将测试代码放入了，这些测试代码必定是高质量的、易理解的，对吗？

错！

测试是一门艺术。它使用了不少的上下文来恰当地平衡风险转移和测试消耗，以适合代码领域和团队文化的方式。Pull request review 是一个很好的、团队能够建立这种共享上下文的地方。

有一些问题须要考虑：

• 测试标题合适地声明了吗？
• 关键场景被捕获了吗？
• 为了安全起见，足够的边缘用例被覆盖了吗？
• 哪部分应用被单个测试使用了？太多？太少？
• 测试断言写得好吗？测试挂过吗？测试常常挂吗？
• 若是一个测试挂掉了，容易追溯到错误缘由吗？
• 若是加入新的前端行为，它有被加入 手动测试脚本 里吗？有被加入 浏览器自动测试 里吗？

7. 不考虑前端复杂性

若是改动发生在 CSS 和 HTML 里，人们的倾向是将它看成算法代码改动来对待。你会看到看起来很规范的改动，而且想象它们会在浏览器里作些什么。“看起来很合理”，你说。

可是事情不是这么简单的。用户最终看到的效果来自于你的应用和不一样的渲染引擎之间的复杂交互。

不要只脑补了，把分支 pull 下来。在多种浏览器和屏幕尺寸上试试，由于这种页面改动是很微妙的。就算你是一个专家级前端开发人员，也不要相信你本身可以靠肉眼搞定这种改动。这也是 CodePen 和 the like 存在的意义！

8. 狭窄眼界

这是另外一个你可能会被 diff 里看起来很规范的代码所麻痹的领域。从大的角度来考虑问题是很重要的。有了项目里的这段新代码，会有什么改变？可能会发生什么？

有一些你能够着手的问题：

• 这个改动会影响用户的下载量吗？对于性能的影响有多大？会改变用户体验，以致于须要放入版本的发布说明，或者放入给用户的邮件里吗？
• 它会引入一种新类型的代码或者特性吗？它须要新的测试方法，新的日志方法、监控技术，或者须要部署流程的改变吗？
• 它会被用户今天用到吗，或者它在一个 flag 后面？存在什么系统能够检测 flag 后面的东西呢？
• 测试彻底有多难？在开发环境和生产环境里可能会有什么区别呢？

9. 短视思惟

在一些 pull request 里，有一些总在反复的东西，多是由于不一样意，或者只是须要阐明。这种作法很棒——这是在创建共享上下文。可是当下一个开发者遇到这段代码的时候，会发生什么呢？他们会对这段讨论难以理解。

为将来创建能够理解的上下文，有这么些想法：

• 在代码的注释里放入关键的 pull request 讨论。
• 改掉对于 reviewer 来讲难以理解的代码——这些代码在将来对于其余人来讲一样会难以理解！
• 在项目里建立一个存放完整的概念文档的地方，包括一些涉及到的、普遍应用的主题。
• 确保全部代码里的 TODO 与你的工做项数据库中的某一项相对，而且有足够的细节能让它被原始报告人之外的人操做。
• 当 review 的时候，请考虑对代码的长期维护——改变会简单吗？在生产环境中维护简单吗？长期消耗是什么？

10. 对修正进行 review

终于！这个 pull request 看起来引发了一些注意，而且又回到了提交者这边。你已经给出了你的反馈，提交者正在相应地做出更改。

如今不是忘掉这个 pull request 的时候。你已经跟提交者讨论好了有哪些须要更改的地方，可是这不意味着这些更改将会是对的！或者甚至彻底是错的！

对 pull request 的修正是开发者有史以来可能作出的风险最大的更改，由于全部人都只想前进。若是一我的在开发中给予的关注不够，那么对 review 也不会太关注。

尤为要密切注意对最初的 pull request 的任何更改，即便你已经完整地 review 过 pull request。若是新的更改被放到了单独的提交里，那么状况要简单些。若是整个 pull request 被从新 rebased/squashed，那么这就让人有点沮丧了。

这不容易！

设计与实现软件是一件难事。凭什么 review 它就会简单一点呢？实际上，review 很大可能更难，由于比起写代码来讲，review 可以用来创建正确的代码背景，从而提供合理的反馈的时间更少。

可是咱们不能放弃——它很重要！

将本文做为你 review 的入口清单吧，或者让它在这方面激励你。随着时间的推移，你和你的团队将会建立一个本身独有的清单，用于提醒 review 代码时一些重要可是容易忘记的点。最终，你的 pull request 流程将会变成一个强有力的 反馈环，可以提高大家团队的 review 文化和代码质量。

掘金翻译计划 是一个翻译优质互联网技术文章的社区，文章来源为 掘金 上的英文分享文章。内容覆盖 Android、iOS、React、前端、后端、产品、设计 等领域，想要查看更多优质译文请持续关注 掘金翻译计划。