拉取请求的争议艺术

这属于那种“我们不需要专门写一篇博文来讨论这个”的话题。但遗憾的是……我真的觉得我们需要一篇博文来讨论这个问题。我指的是围绕代码审查和拉取请求的现代实践。

一点背景

我年纪够大了，还记得以前用打印稿、投影仪，或者（我的天哪）白板进行的代码审查。这些​​审查不仅在策略上难以管理，而且在实际代码质量方面也非常不切实际。因为等到有人进行“正式”的代码审查时，代码通常已经基本完成了。

所以，等到团队其他成员看到它的时候，我们或许能提供一些有用的建议，以便在最后关头进行清理。但要纠正那些本应在新模块/功能/等等中实施的大规模改动几乎是不可能的。

即使使用早期的源代码控制工具（例如 Visual Source Safe 或 SVN），也很难真正监控团队其他成员的工作进度。即使你设法监控他们的代码，也往往是在代码部署到生产环境之后。

毫不夸张地说，这git代表了该领域的一次巨大飞跃。如今，你不必使用……git但我们大多数人还是会用。我真心认为，这git为持续进行代码小组审查提供了最佳范例之一。即使你使用的是其他源代码控制工具，拉取/合并请求的流程也可能与我们目前在……中的做法非常相似git。

代码协作日趋成熟

如果你在协作编码环境中工作，拉取请求是最强大的工具之一，它可以确保每个人都能看到（或至少有机会看到）其他人提交的内容。理想情况下，在代码合并到父分支之前，你的部分（或全部）团队成员应该仔细阅读大部分（或全部）代码。

这功能太强大了，太棒了。我真不想回到没有这么便捷的实时工具来查看和评论别人代码的时代。

但就像所有美好的事物一样，即使是拉取请求也有缺点。

讽刺与迂腐的论坛

开发人员，作为一类人，以有点……刻薄而闻名。如果我们是优秀的“人际交往高手”，我们就不会选择那种大部分时间都盯着屏幕的职业。戴着耳机隔绝所有环境噪音（以及任何与他人的“真实”互动）。

开发人员有时也会非常……固执己见。（震惊！！！）非开发人员甚至无法理解像制表符和空格、前逗号和尾逗号、类和函数这样“琐碎”的事情所引发的“圣火”。但开发人员对这些争论早已了如指掌。他们坚定地站在某一方。如果你敢提交不符合他们内心偏好的代码，他们很可能会把你骂下去。

虽然我非常推崇拉取请求流程，但我也必须承认，我见过它出过太多大问题。太多次了。当你邀请另一位开发人员查看你的代码时，你几乎能听到他们捶打指关节、深呼吸，准备迎接一场“Snark Ba​​ttle”的声音。

所以，尽管我真心希望这篇文章毫无意义，但我意识到我已经积累了一套自己的内部“拉取请求规则”。我真心希望，在某个地方，你们中的一些人能读到这些规则——并且至少把其中的一些牢记在心。

规则 1：Linting 并非“可有可无”

我在这个网站上的第三篇文章完全是关于 ESLint 的。（你可以在这里阅读：https://dev.to/bytebodger/why-i-hate-eslint-and-how-i-learned-to-love-it-40bh）具体来说，我承认我最初讨厌linting 工具。它们就像一个烦人的保姆，一直在我身后盯着，告诉我我选择的编码风格有点“不对劲”。

如今，我爱上了linting 工具。我之所以“转向” linters，并不是因为我喜欢（甚至同意）它们强制执行的那些繁琐规则。我之所以依赖它们，是因为它的拉取请求流程。

规则 1：所有关于代码样式的琐碎细节都应该在 Linter 中定义/强制执行。如果你编写的代码不符合某人对“良好代码风格”的内在要求，但却通过了 Linter，那么在拉取请求过程中就不应该再争论这个问题了。

你看，一个好的 linter 应该是你团队中抵御任何代码纳粹的首要防御措施，这些纳粹会因为一些繁琐的样式细节而阻止你的拉取请求。

你的团队想要空格覆盖制表符吗？那么最好在 linter 中强制执行。

你的团队是否希望将左花括号单独放在一行？那么最好在 linter 中强制执行。

你的团队是否希望case语句在代码块内缩进switch？那么最好在代码检查器中强制执行。

如果您的团队仍在尝试通过政策“强制执行”代码样式标准，并且不愿意花时间实现共享的 linter，那么......欢迎来到 2002 年。您的 Nickelback 音乐会 T 恤正在耐心等待您。

我最近在一个团队里工作，他们觉得自己太忙了，没时间实现任何代码检查工具。但他们内部仍然有一套杂乱无章的“编码标准”，觉得必须强制执行。即使他们所谓的标准在他们自己的代码库中不一致，也没关系。我写完代码，提交拉取请求，然后他们就会对我进行一系列的风格修改，要求我修改后才能合并。

当涉及到代码风格决策时，问题应该非常简单：它通过了 linter 测试了吗？如果它通过了 linter 测试（或者你懒得实现linter测试），那么将拉取请求流程作为一种随意的手段来强制执行你定义不明确的“标准”是一种极其糟糕（且具有对抗性）的做法。

如果我的表达不够清楚，如果你觉得有必要在某人的拉取请求中乱扔一系列命令来改变编码风格，那么你就是一个拉取请求混蛋。

规则二：谦逊

我见过太多次，在 PR 评论中，审阅者自信满满地宣称某些内容“错误”或必须修改。即使在以自我为中心的应用程序开发领域，这种不容置疑的声明也常常存在缺陷。而且，它们很少能培养团队成员之间的良好合作精神。

规则二：如果你在别人的拉取请求上发表大胆声明，你最好确保自己是对的。即便如此……你也不应该在别人的拉取请求上发表如此大胆的声明。

保持谦逊一点。即使对于最资深的开发人员来说，仅仅看一眼代码片段并真正理解其中发生的一切——或者为什么做出特定的编码选择——也可能极具挑战性。审阅者至少有可能没有完全理解代码中发生的事情。

考虑一下这两个示例 PR 评论之间的区别：

Are you sure that you want to use a regular expression here?  
It seems this might be more efficient with a simple string comparison.

对比

The RegEx used here is inefficient.  Change it to a simple string comparison.

第二句话真是……自信爆棚，甚至可以说是傲慢。第二句话更有可能引发代码提交者的防御性反应。

[注：我甚至不想讨论你是否正确。在上面的例子中，使用正则表达式完全有可能效率低下。也有可能程序员真的应该把代码片段改成使用简单的字符串比较。但这不是重点。 ]

谦逊地表达你的反馈通常会在 PR 上得到更好的回应。这将增进所有代码审查者（包括但不限于原始程序员）之间的相互尊重。

而且无论你自认为多么自信，通常至少存在某种可能性，你实际上是错的。或者，至少，最初的程序员有充分的理由以那种方式编写代码。

问题促进讨论，​​陈述促进辩论。

即使在极少数情况下，我确信自己绝对正确，我也很少在 PR 上添加不可侵犯的声明。例如，我见过一些代码根本无法编译的PR 。我是否通过声明某些地方出了问题来羞辱他们？没有。我通常会留下这样的评论：

It doesn't look to me like the line above will compile.  Am I missing something?

通常，在这种情况下，我知道我没有错过什么。但我发现，如果只是简单地把评论换成一个温和得多的问题，通常会带来更强烈的持续合作感。

规则3：强力评论

PR 评审的重点在于鼓励尽可能多的团队参与整个过程。彻底破坏这种参与度的一个方法就是完全忽略某人的评论，直接合并代码。

规则＃3：在请求的所有评论都得到答复之前，不应合并任何拉取请求。

[注意：我并不是说每条评论/问题都必须得到令你满意的答复。在高速发展的环境中，如果有人这样回答你的评论/问题，有时甚至是可以接受的：“是的——这需要重构，但现在我们只需要将其合并，以便 QA 可以开始一些测试流程。”]

没有什么比你花了时间仔细审查并评论某人的 PR 更令人不安的了，几个小时后，你却发现它被合并了，代码没有任何修改，评论也没有任何回复。这种情况，我称之为“强行评论”。

最近，我在一个团队工作，团队的技术主管抱怨他们的 PR 流程“支离破碎”，而且大多数开发人员并没有真正参与其中。在那里工作了仅仅几周后，我痛苦地意识到了为什么会出现这种情况。

除非你是他“选定的开发者”之一，否则你可以仔细检查/评论/质疑这个请求——它仍然会被合并。没有任何回应，代码也不会有任何修改。没过多久，团队里的新开发者就意识到，费心审查别人的代码纯粹是浪费时间。他们要么直接盲目批准，要么干脆直接忽略这个 PR。

规则4：代码雪崩

前面的规则都是针对代码审查人员的。但在有效的审查过程中，代码提交者也承担着责任。程序员犯下的最大错误之一就是“代码雪崩”。

规则＃4：拉取请求应始终尽可能小。

说实话，我们偶尔都会犯这种错误。我们在本地主机上开发下一个伟大的功能。它需要几十个新文件，或者对现有文件进行大规模修改。它包含数千行代码。出于各种原因，我们一直把它藏在自己的私有分支里，从未推送到其他开发者能看到的地方。

当我们终于准备好将这个庞然大物部署到某个底层环境时，我们会创建“地狱般的拉取请求”。我们会将所有曾经接触过代码库的人添加为审批人。然后我们焦急地等待着他们的“橡皮图章”式批准。

坦白说，当我们犯下这个错误（以及这段代码）时，我们内心深处都清楚自己在做什么。这就像一场巨大的认知倾泻，没有人能够指望读完我们写的所有东西并提供任何有意义的反馈。我们暗自希望有人能被所有的改动淹没——然后……批准它。

光是阅读代码就已经很困难了。当代码不在本地运行，而是直接“打印”在屏幕上时，指望任何人真正理解其中的逻辑，这本身就是一个很大的挑战。当你要求一个人审查数千行新增/更新的代码时，这个挑战的难度会成倍增加。

当你把一堆代码倾倒给别人时，他们或许能发现代码中的一些小瑕疵。但认为他们能提供更深入的反馈来判断你的整体方法是否“有效”是荒谬的——因为他们甚至很难理解你的新代码究竟要实现什么更广泛的目标。

规则五：时间人质

如果你真的关心代码审查流程，那么你应该知道任何有意义的审查都需要时间。它不应该花上几天时间，也很少会花上几个小时。但它确实需要时间。你见过多少次这样的电子邮件/聊天消息？

Hey.  I just submitted this pull request. Can someone please 
review/approve it?  I'm trying to make the automated noon deployment 
to QA.

然后你低头看了看手表......现在是上午 11:55。

规则＃5：编码员/提交者始终有责任确保所请求的批准者有合理的时间对代码进行善意的审查。

这又是一个“我们有时都会犯错”的问题。我明白。但承认我们自己也犯了错，并不能让问题变得更好。如果你站在我身后（无论是虚拟的还是现实的）乞求我的认可，而我的时间只有几分钟，那么恭喜你。你正式地试图把我变成“时间人质”。

当然，也有很多例外。如果我确信我“批准”的代码在目前状态下永远无法顺利投入生产——那么，是的……我可能会很乐意批准它。但如果这段代码真的在快速部署中，那么我不会接受你任何最后一刻的绝望请求。

规则#6：责任

如果说公关流程中有什么环节经常“出问题”——在我工作过的很多开发公司里——那就是故意缺乏问责制。我指的是什么呢？

好吧，当你批准别人的拉取请求时，你绝对应该将其视为你个人对该代码的认可。在极少数情况下，一些真正“糟糕”的代码最终进入生产环境，我（不幸地）见过发起该代码的开发人员被直接扔到公共汽车下。但我很少看到有人问：“但是谁批准了这段代码？”

规则六：拉取请求的批准绝不能被视为理所当然。这是一份个人声明，表明至少有一位其他开发人员审查了你的代码，并且他们对此负责。

[注：我并不是说你的“批准”意味着你绝对喜欢这段代码——或者它试图实现的目标。我的意思是，任何批准者都应该对任何未经检查的严重错误承担至少部分责任。]

冒着法律风险，我想指出“责任”和“责任”之间存在关键区别。如果乔设法将一些糟糕的代码投入生产环境，并最终导致整个网站/应用程序瘫痪，那么最终谁应该为这一失败负责？当然是乔。我甚至可以接受乔可能会受到纪律处分——或者在最坏的情况下被解雇。但如果鲍勃批准了该代码，那么他也应该至少在某种程度上为这一失败承担责任。

这在开发团队里是个非常敏感的话题。一方面，我们谁也不想承受 Joe 好不容易才把那些烂代码投入生产环境的后果。另一方面，如果拉取请求的批准者完全不用为他们的行为承担任何后果，那他们为什么还要在“批准”代码之前对它进行任何有意义的审查呢？

我亲眼目睹过这种情况——无论是积极的还是消极的——在实际工作中。我曾经在团队里工作过，没人敢问：“等等……这段代码是谁批准的？”而且在那些团队里，审批成本很低。

我也曾在一些团队工作过，他们的审批确实承担了一定程度的责任。你知道吗？在这些团队里，审批流程变得意义非凡，这真是令人惊讶。当人们知道他们的审批理论上可以在事后接受审查时，他们就会神奇地停止“橡皮图章式”地批准拉取请求。他们会开始仔细关注任何他们认可的提交。

如果这听起来有点像“猎巫行动”，那绝对不是。在我上面描述的场景中，糟糕代码的作者（Joe）应该永远是承担其行为最大后果的人。但如果Bob至少没有被问及他对那段糟糕代码的批准，那将是一个可怕的错误。

我还想明确一点，我所说的责任只应在出现极其严重的错误时才被追究。bug难免会发生，我理解。而且，没有人应该仅仅因为在拉取请求中浏览了代码，就指望他们能发现所有可能存在的 bug 。

但如果提交的内容（但愿不会发生，甚至部署到生产环境）构成了明显错误，那么该 PR 的审批人至少应该受到质疑。否则，整个团队的质量都会受到影响。因为拉取请求的审批流程将变得毫无意义。

规则7：禁令之锤

如果您确实认为某个拉取请求是错误的，那么您什么时候才会真正拒绝它呢？

噢，天哪……

如果你觉得拉取请求问责制是个敏感话题，那就等着瞧吧，拒绝拉取请求会带来哪些政治陷阱。对很多开发者来说，拒绝别人的拉取请求会让他们很不舒服。这本不应该。但说实话，这绝对是。

那么，您该如何避开这个雷区呢？

规则 7：拒绝拉取请求是最后的手段。但避免使用最后手段绝不应该成为批准的借口。

首先，我们得承认，拒绝某人的 PR有时会让人产生某种情绪。理想情况下，我们都不会对自己的代码投入如此多的情感。但现实远非完美。而且开发者的性格有时……很敏感。拒绝 PR 通常会让请求者感觉像是给他们的作品打了个“F”。

其次，让我们评估一下您真正拥有的所有选择。

1. 只要你的团队不倾向于强制评论，评论过程本身通常比直接拒绝某人的 PR 更可靠。在我上面举的例子中，如果你写了这样的评论：“我觉得上面这一行代码编译不了。我是不是漏掉了什么？”，这应该足以阻止代码合并，直到它被“修复”，或者你收到更强有力的保证，证明它确实能编译通过。因此，拒绝某人的 PR 通常是一种糟糕的做法，前提是你已经通知了请求中的其他人你遇到了问题——并且应该得到解决。
   
   

2. 通常情况下，你不需要参与审批流程。这在规模较小的团队中可能会有点问题。但假设这个 PR 上还有其他请求审批的人，那么“袖手旁观”完全没问题。我曾经遇到过这样的情况：有人直接过来让我审批一个 PR，我看了看代码，觉得代码太烂了，就直截了当地告诉他们，我不会批准这个请求，但也不会拒绝。换句话说，我基本上是在告诉他们，我不会因为批准这个请求而承担责任，但如果其他人愿意，我也不会阻拦。
   
   

3. 删除拉取请求可能比拒绝拉取请求要好得多。例如，假设您在拉取请求中发现了一个彻头彻尾的编译错误。您确信如果部署了它，它根本无法运行。但请求者目前不在办公桌前，您担心如果您袖手旁观，其他人可能会误批准该拉取请求，并将其部署到其他环境中。在这种情况下，我经常会在拉取请求上添加一条评论，表明我的担忧，然后将其删除。即使该请求已被删除，原始提交者仍然应该收到您在删除之前留下的评论的邮件通知。如果出于某种原因，他们没有收到您的评论通知，他们可能会过来问：“嘿！你们为什么删除我的拉取请求？？？”这听起来可能和拒绝请求没什么区别。但根据我的经验，令人惊讶的是，开发人员在发现自己的请求被删除时，比被拒绝时更加坦然。
   
   

4. 并非所有沟通都必须在拉取请求中体现。在现代社会，我们有时会忘记这个简单的事实。我们想当然地认为，如果 Joe 写了一些“超级糟糕”的代码，我们别无选择，只能在拉取请求中指出。但这对提交者来说，就像是一种“代码羞辱”。有时，我们甚至觉得必须拒绝请求。但很多时候，我会直接走到Joe 的办公桌前，或者在 Slack 上联系他，然后说：“嘿……我看到了你最新的 PR。但我觉得有些地方不太对劲。你真的是想提交这个吗？？”在这种情况下，我经常发现，这种简单的个人沟通比直接拒绝PR有效得多。如果你遇到代码雪崩（Code Avalanche），这种做法也会很有帮助。在这种情况下，我有时会把开发人员叫过来，说：“帮我解释一下。”

结论

我相当肯定我漏掉了一些关键点。哎呀，说不定哪天就能写出第二部分了。

这篇文章可能感觉充满了“大胆”（甚至是傲慢）的言论。我真心觉得软件开发中有很多方面并不适合用硬性“规则”来规范。但就我在本文中概述的内容而言，我对此深信不疑。要让我相信这些规则中的任何一条实际上都是有缺陷的，那可不是什么容易的事情。

但是……我以前也犯过错（据说）。你怎么说？