尚学堂程序员对合并请求的 10 个常见问题及建议

1.不佳思索的 zan 同

事情就是这么有诱惑力。这次的合并请求规模真的很大，提交者又是你信任的某个人。他们已经在这份代码上工作一段时间了，而且一直都处理的很好。不要说你还有自己的截止日期（deadline）要跟。

振作起来吧！

你需要真正花时间去审查那份代码。每个人都会犯错——再有资历的人也没有魔法棒。而你作为一个审查者，你的工作就是用你的创造力和专业知识减少这份合并请求以任何方式让代码库变得更糟糕的概率。

这才是真正的目标，为什么不是呢？如果每个合并请求都能让代码库更好，那这个项目就确实有长期发展的潜质。

2. 拖延

毕竟是一个这么大的合并请求，为什么非要现在审核？而且你现在的任务实在太重要了。你最终会抽时间来处理它的，是吧？或者你可能只是在等其他人发表意见……

扪心自问，让原力贯通全身！在你内心抵制的背后可能会有一些真实的抱怨。

既然你已经明确了你的关注点，那就开始行动吧！

· 如果提交者没有提供足够的引导告诉你这些修改都做了哪些事，那就去跟他要！例如，最初的需求文档在哪里？

· 如果这些修改内容太多了一次审查不完，那就让他们分批提交！

· 如果出现你不理解的东西，那就要让他们给你讲清楚！

· 如果你发现一大堆问题/关注点，那可能就是需要和提交者面对面沟通的时候了。

3. 标准 diff

你是否正在审查一些乱七八糟的代码？GitHub 和企业版 GitHub 的默认 diff 工具是“标准 diff”。这种模式下，为了把一系列的修改都表示在一个文件中，GitHub会关注增加和删除的行并很智能地将修改编成组块，而且这些都是行内表示。但是你知道吗，大多数情况下，“标准”diff是很难阅读的。所谓智能的组块选择其实一点也不智能。

好在 GitHub 和企业版 GitHub 现在都支持“分片”diff。左边是旧文件，右边是新文件。如果删除代码那右边就是空白，如果增加代码那左边就是空白。不论哪种，你都可以清晰地看明白这文件之前和之后是什么样子，有助于做出更好的审查决策。

不要去解决混乱。点击 diff 页面右上角的“Split/分片”。

4. 风格盖过本质

审查合并请求的时候除非必要请花尽量少的时间去讨论代码风格和格式。我之前写过关于使用 ESLint 这类工具把这些事情完全自动化的必要性。为什么呢？因为浪费时间！

合格的代码审查者应当把时间花在理解这些代码修改的根本目的，就是去回看最初的需求。这对应哪一条工作项？这是一个特定处理？它到底想要完成什么？

只有了解上下文环境才能做到真正的代码审查。有些在审查表面结构／风格的时候，看起来合理的地方在你理解了终极目标之后可能就变得不可接受了。

好吧，现在你可能想要回避提出这么大一个问题，因为已经在现在的代码更改上花费太多时间了，但讨论出一个更好的解决方案是很值得的。这对每个人都是一个学习机会。也可能你觉得会有更好的解决方案这个想法就是错误的，但你需要和最初的提交者讨论之后才能发现这个事实。

5. 没有捕获不完善的修改

diff 工具确实能很清晰地告诉你哪些地方有修改。但这也是问题所在！显然它们没告诉你哪些地方没有改变。要当心有些本应该应用更广泛的修改，像 find/replace 这类的可能没有覆盖整个代码库。

或者说是一个修改只完成了它本应修改的组件的子集。

或者根本就没有测试。测试对于任何修改都是很重要的部分，但如果测试不出现在diff工具里，确实很容易忘记。而且也不会有人提示你想起来测试。

不得不承认，这真的很难！这是代码审查中最难的。在提交者的分支或是你自己电脑的分支上做一些快速健康检查可能会有些帮助。或者你可以咨询提交者他们除了你能看到的代码之外还做了哪些综合详尽的测试检查。

6. 粉饰太平的测试代码

合并请求里只要有一些测试代码，很容易就让人产生这很安全的错觉。如果他们把测试考虑进去，那他们一定是高质量且综合全面的。是这样的吗？

错！

测试是一门艺术。恰当地平衡风险规避和测试代价以及是否适合代码覆盖范围和团队文化。审查合并请求是团队建立共享背景的绝佳时机。

还有一些问题需要考虑：

· 测试标题能否充分描述目的？

· 一些关键场景是否能捕获？

· 是否有足够的边界测试样例？

· 一个测试会执行应用程序的哪些模块？太多？或者太少？

· 测试是用恰当的断言写的吗？是否失败过？会经常失败吗？

· 如果测试失败，能否快速追踪错误？

· 如果有新的前端行为添加，会被加进手工测试脚本吗？看看自动测试？

7. 低估前端的复杂度

如果 CSS 和 HTML 有修改，会倾向于把它看作和算法代码更改一样。看到格式良好的修改就会想象他们在浏览器里如何执行。“看上去很合理，”你说。

但是事情不会这么简单的。最终用户看到的结果来自你的应用和各种渲染引擎的复杂交互。

跳出固有思维，把分支拉取下来。在各种浏览器和不同的屏宽比里尝试。因为这些事情真的很容易迷惑人。即使是资深的前端开发人员，也不要完全信任自己一直盯着的事情。这也是 CodePen 和 the like 存在的原因！

8. 思维狭隘

还有一个地方可能会因为 diff 里格式良好的代码让你昏昏欲睡。但考虑大规模情况是很重要的。目前项目里的新代码，到底有什么变化？会发生什么？

可以从这些问题开始思考：

· 这会影响用户的下载量吗？会感知到性能变化吗？对用户体验的改变是否足以需要在版本注释里说明，或是给用户一封邮件说明？

· 它是否会引入新的代码或特性？是否需要新的测试方法、新的日志或监控技巧，或者会改变部署过程？

· 用户今天能否执行，或者需要等到一个标记信号之后？什么系统能够原地验证标记之后的事情？

· 综合完善的测试需要多大难度？预发环境和线上环境到底有什么区别？

9. 短期思考

有些合并请求里有相当多的往返，可能是因为意见不一致或者是阐明的需要。这确实是一件好事情—它在建立共享上下文环境。但是当下一个开发人员来了并拿到这份代码会发生什么呢？他们可能就没那么容易理解这场讨论了。

未来建立可理解的上下文环境的一些建议：

· 在代码注释里充分体现关键的合并请求争论。

· 修改审查者很难理解的代码—其他人未来也一样理解不了！

· 在项目里创建一个存放概念文档的地方，这个文档覆盖更多相关的、广泛应用的话题。

· 确保代码里所有的

TODO

和工作项集合里的每一项都是对应的，要足够详细到让除了最初报告人之外的人也能执行。

· 代码审查的时候，考虑代码的长期维护—是否以后也能易于修改？线上环境是否易于维护呢？长期维护的代价是什么？

10. 审查修正的时候草草了事

终于，你集中注意力完成了这个合并请求，并将它返回给提交者。也给出了反馈，提交者回应正在修正。

现在不是忘掉这个合并请求的时候。你确实是已经和提交者讨论完需要的修改了，但不意味着这些改变都会是正确的！或者说全部都能修正！

修正合并请求是开发人员所做的风险最高的修改，因为每个人都想赶紧继续下一件事。开发过程的关注越少，审查过程给的关注也就越少。

好吧，即使你已经非常详细的审查过这个合并请求了，但还是要尤其注意最初合并请求里的任何更新。如果新的修改被划分进一个单独的提交，那还好办。如果整个合并请求都被重新复位/压缩，那就让人头疼了。

这不是一件简单的事情！

设计并实现软件是一件很难的工作。那为什么审查会更容易呢？事实上，审查可能会更难，因为你要用少得多的时间去建立一个正确的上下文环境以提供合理的反馈。

但我们也不能放弃审查，因为它实在太重要了！可以把本文当作一个起步的检查表，或者以此为灵感建立一个新的检查表。久而久之，你和你的团队就可以给那些很重要但容易被忽视的考虑点建立一个定制的备忘录。最终，你们的合并请求过程会形成一个强大的反馈循环，能够提升团队文化和代码质量。