Google 是如何做 Code Review 的?| 原力计划( 七 )


如果CL中引入了新的复杂性 , 提交之前必须清理掉 , 除非是紧急情况 。如果CL中暴露出一些目前还无法定位的问题 , 开发者应该记录下这些bug , 并将其分配给他们自己 , 确保这些问题不会被遗忘 。他们还可以在代码中加入 TODO 注释 , 指向已经记录好的 bug 。
抱怨太严格
如果你之前Code Review很宽容 , 然后突然变得严格起来 , 可能会引起一些开发者的抱怨 。不过没关系 , 加快Code Review的速度通常会让这些抱怨消失 。
有时 , 这些抱怨可能需要几个月的时间才能消除 , 但最终开发者在看到产出的优质代码时会理解严格Code Review带来的价值 。有时候 , 一旦发生某些事让他们真正看到严格Code Review的价值 , 抗议最大声的人甚至会成为你最坚定的支持者 。
解决冲突
如果你遵循了上述方法 , 但仍然会在Code Review中遇到无法解决的冲突 , 请再次参阅Code Review标准 , 了解那些有助于解决冲突的指导和原则 。
3.发者指南写一个好的CL描述
一个CL描述是做了什么更改以及为什么的公开记录 。
它将成为我们版本控制历史的永久组成部分 , 多年来除你的评审者以外 , 还可能有数百人会阅读它 。
将来的开发者将会基于它的描述来搜索你的CL 。
将来可能会有人由于没有现成的细节 , 而根据他模糊的记忆来寻找你的改变 。
如果所有重要的细节都在代码中而不是描述中 , 那么他们将很难定位你的CL 。
第一行

  • 言简意赅
  • 语义完整
  • 空行隔开
CL描述的第一行应该是对CL正在做的具体工作的简短总结 , 紧跟一个空行 。在未来 , 这是大多数的代码搜索者在浏览一段代码的版本控制历史时会看到的 , 因此第一行应该足够有信息量 , 他们不必阅读你的CL或它的整个描述就可以大致了解你的CL实际做了什么 。
按照传统 , CL描述的第一行是一个完整的句子 , 就像它是一个命令(一个祈使句) 。例如 , 说"Delete the FizzBuzz RPC and replace it with the new system." , 而不是"Deleting the FizzBuzz RPC and replacing it with the new system." 。
不过 , 你不必把其余的描述写成祈使句 。
正文内容丰富
其余描述应该是具有丰富的内容 。它可能包括对正在解决的问题的简要描述 , 以及为什么这是最好的方法 。如果这种方法有任何缺点 , 就应该提到 。将相关的背景信息(例如bug数目 , 基准测试结果) , 以及设计文档的链接 。即使是很小的CL也值得关注细节 。请将来龙去脉放入CL中 。
反例
"Fix bug"是一个不充分的CL描述 。是什么bug?你是怎么修复它的?
其他一些类似的不好的CL描述:
  • “Fix build.”
  • “Add patch.”
  • “Moving code from A to B.”
  • “Phase 1.”
  • “Add convenience functions.”
  • “kill weird URLs.”
这里有一些是真实的CL描述 。他们的作者可能认为他们提供的信息是有用的 , 但他们并没有给出一个CL描述的意图 。
好的CL描述
这里有一些好的CL描述的例子 。
功能改变
rpc: remove size limit on RPC server message freelist.
Servers like FizzBuzz have very large messages and would benefit from reuse.
Make the freelist larger, and add a goroutine that frees the freelist entries slowly over time, so that idle servers eventually release all freelist entries.
前几个词描述了CL描述实际做了什么 。其余描述在说明解决的问题 , 为什么这是一个好的方案 , 以及具体实现的细节 。
重构
Construct a Task with a TimeKeeper to use its TimeStr and Now methods.
Add a Now method to Task, so the borglet getter method can be removed (which was only used by OOMCandidate to call borglet’s Now method). This replaces the methods on Borglet that delegate to a TimeKeeper.
Allowing Tasks to supply Now is a step toward eliminating the dependency on Borglet. Eventually, collaborators that depend on getting Now from the Task should be changed to use a TimeKeeper directly, but this has been an accommodation to refactoring in small steps.


推荐阅读