代码审查

代码审查的总体目标是作为团队其他成员的安全网，帮助他们编写更好的代码，而不是评判他们或他们的代码。如有疑问，请假设他们是出于好意，并友善对待。

目标

• 发现错误
• 发现某种方法不明显的后果——此 PR 会不会导致未来的代码更难保障安全或更容易出现错误。
• 对于未经讨论就编写的代码，代码审查可作为一种健全性检查，以确保采用了正确的方法。
• 指出此 PR 对 Metabase 未涉及部分的影响
• 指出使用了良好方法或风格的地方。代码审查不是一场批判大会。除非 PR 完全糟糕透顶，否则应同等数量地提出优点和缺点。

进行代码审查的心态

作为审查者的主要目标是充当安全网，防止糟糕的代码被合并。“糟糕”的定义高度主观，取决于上下文，并将随着产品时间和成熟度而变化。

当您发现明显错误时，花时间注明您认为它们是错误的原因。

如果您发现不认同某个方法的地方，请提出。但是，也要花时间了解作者做出特定选择的原因。您应该假设作者当时根据他们所知做出了正确的决定。您可能拥有不同的知识体系，并看到不同的结果。深入探究这些问题。他们可能看到了您没有看到的东西，反之亦然。

寻找您可以学习的技巧、技术或惯用表达。您的队友都很聪明。他们很可能有一些您可以学习的技巧。务必让他们知道。

接受代码审查的心态

审查者是在帮您的忙。他们是为了帮助您尽力完成工作。最优秀的人都有教练、编辑和导师。您的代码审查者也应该以同样的方式帮助您。在他们经验更丰富的情况下，这可以是直接指导。在他们经验较少的情况下，他们有一双全新的眼睛，可能会让您质疑根深蒂固的假设。

当审查者不同意您所采用的方法时，请寻求理解原因。他们可能知道您不知道的事情或看到您没有预料到的后果。虽然他们可能没有像您一样深入思考 PR 的具体主题，但他们同样可能在考虑 PR 对您可能没有注意到的领域的影响。

如果有人对您的 PR 投了强烈的 :-1:，请特别耐心。深入探讨他们认为 PR 有缺陷的原因。带着改进 PR 的意图进行对话，而不是为自己的方法辩护。您不会因为擅长辩论而得分，但无论灵感或想法来自何处，您都会因为交付更好的代码和更好的产品而得分。

流程

• 每个复杂程度较高的 PR 都需要团队中至少一名其他工程师（或 @salsakran）给予 :+1: 才能合并
• 将您认为应审查您 PR 的人员添加到 PR 的受让人中。审查者在审查完毕或认为自己不是合适的审查者后，可以自行移除。
• 影响其他工程师工作的代码应由这些工程师进行审查
• 一个 :+1: 是默认的“我同意”
• 一个 :+0:（我编造的）是“我对此并不十分满意，但其他人说‘+1’就意味着可以合并”
• 一个 :-1: 是强力否决。这应在普通 PR 中谨慎使用，且仅用于缺少测试、公然违反样式指南或破坏代码库其他部分所依赖的假设的情况。
• 如果您在未讨论设计或未与其他可能受影响的工程师讨论其影响的情况下切出一个主分支，您应该预料到会收到一个 :-1:，并且不要执着于重做有争议的部分。
• 任何带有 :-1: 的 PR 在解决之前不能合并。
• PR 的所有者和投 :-1: 的人应该解决方法上的分歧。
• 如果出现僵局，@salsakran 将投下决定性的一票。僵局应该很少发生。

请注意，这些 :+1:、:+0: 和 :-1: 应在评论中明确说明，而不是在 GitHub PR 的主要描述中作为反应。从 :-1: 到 :+1: 的更改也应在评论中明确说明。

时间安排

• 高优先级问题的 PR 应在可用后立即进行代码审查。
• 里程碑中问题的 PR 可以等几天。
• 如果 PR 上没有 :+1:，则 PR 创建者有责任跟进其他人并获得其代码审查。重申一下，PR 需要被 :+1: 才能合并，如果尚未审查，则由 PR 的发起者召集审查者。
• 如果存在 :-1: 且没有明确的解决方案，PR 创建者和投 :-1: 票的人都应计划在未来一两天内花一小时讨论该问题，并规划如何解决。
• 如果带有 :-1: 的 PR 在一周后仍无进展，@salsakran 将介入。

如何提高代码审查的质量

有关代码审查研究的摘要，请查看 代码审查在现实世界中如何运作（和不运作）。

PR 作者如何获得更好的审查

• 通过对代码的重要部分进行评论来指导审查者。
• 如果您需要某个人的专业知识/意见，请标记该人员。
• 通过使用 注释和警告 来增强 PR 描述——如果您希望某些信息突出显示，这些工具会很有效。

PR 审查者如何给出更好的审查

• 密切关注测试文件。请注意我们倾向于降低测试文件重要性的倾向，并自觉努力彻底审查它们。
• 从对更改最重要的文件开始，而不是 PR 中按字母顺序显示的第一份文件。
• 如果您觉得缺乏上下文，请向作者索取更多细节/更好的描述。
• 除非更改微不足道，否则请检出该分支并在本地运行 Metabase，包含这些更改。确保一切按预期工作。