Skip to content

Latest commit

 

History

History
221 lines (148 loc) · 8.69 KB

代码审阅.md

File metadata and controls

221 lines (148 loc) · 8.69 KB
Raw

代码审阅

代码审阅是保证代码满足一定的质量下限标准的关键环节。主要关注无法通过自动化方式检查的清单、规范、标准和流程的执行情况。

为了使做法更具参考价值，我们先将代码审阅局限在每一次代码变更请求即 pull/merge request 环节。对于其他场景如审计等，可以在本文基础上扩展、变通。

以下的内容均基于 github pull request 功能，及与之相关的名词、概念、做法等。基于其他平台、形式的流程可以在本文基础上变通。

参与者与职责

提交者

提交者通常是代码的作者，是最了解代码详情的人。在代码审阅过程中，提交者必须与其他参与者进行讨论、合理回应其他参与者的要求。

提交者同时也是第一审阅者，因此也应当履行审阅者的绝大部分职责，且在对具体内容的审视上，提交者应当执行比其他审阅者更严格的标准。

审阅者

审阅者需要根据自己的专业知识、对现状的了解程度，审视待合并的变动。

在以下方面，审阅者必须确保其符合最低限度的质量标准，我们标记为 p0 事项：

  • 正确性及确保正确性的环节，如测试用例的覆盖范围等
  • 团队内执行的标准

同时，审阅者应当在以下方面执行严格的标准，我们标记为 p1 事项：

  • 能够降低产生分歧的可能性的环节，如模块、类型、值、函数的命名等
  • 能够降低协作难度的环节，如注释内容等
  • 能够降低后续维护复杂度的环节，如 commits 的影响范围等
  • 团队内执行的规范

此外，审阅者可以在以下方面提出建议，我们标记为 p2 事项：

  • 代码编写方式
  • 未作为规范引入的最佳实践
  • 文档的措辞、拼写错误
  • 优化、扩展性或类似的、影响未得到证明的环节
  • 其他不涉及正确性的环节

对于事项划分、和针对不同事项的解决方式，参考 事项规范。

合并者

合并者通常就是最后一位审阅者，也可以是专职的管理者。

合并者必须确保所有阻塞合并的事件被正确解决，如 review approved、checks passed 等。 同时合并者应当在合并完成后执行适当的清理工作，如源分支的清理等。

其他参与者

其他参与者是指在代码审阅过程的某个局部环节，被提交者或审阅者引入的第三方。

其他参与者应当履行知情、反馈、提供必要协助等职责。

注意，审阅者因对代码整体产生质疑——如设计思路、实现方式等——而引入的第三方，应当视为新的审阅者。

流程

  • 此流程在 pr 开启至 pr 被 merge/close 的过程中可能会多次循环/中止。
  • 任何一个提交 review approval 结果的审阅者，都必须确保以下内容被完整检查，出现的问题都被解决。

基础信息 & Conversation

  • 检查 head branch(pr 的源分支) 和 base branch(pr 的目标分支)；p0

  • 检查关联的 issues：

    • 联动语法是否正确；p0

      参考 Linking a pull request to an issue using a keyword；

      这是为了避免出现代码合并而关联issue未关闭的情况；

      注意：联动的 pr 只有在同时满足：

      1. 目标分支为当前代码库默认分支
      2. 联动文本位于 PR 的 description（创建 PR 时的描述信息；PR 合并前，通过 edit 更新有效）中时才能正确地联动关闭。

    • issue 涉及的范围和本 pr 解决的范围是否一致；p0

      这是为了避免只解决部分，甚至完全未解决issue却联动关闭的情况

    • 关联的 issue 是否全面；p2

  • 检查是否存在未解决的 conversation；p0 解决方式和解决者参考 事项规范-解决

Commits

  • 确认 commits 涵盖的内容与 pr 要解决的问题匹配；p0
  • 确认 commits 和 pr 的粒度划分合理；p1
  • 确认 commit messages 符合规范；p1 参考 commit-message风格规范

Checks

Files Changed

以下会针对不同类型的内容，罗列常见的审阅项清单，实际审阅过程中，可能不局限于以下罗列的项目。

审阅过程中对于辅助工具的使用可以参考 审阅辅助工具使用规范。

代码

  • 实现
    • 是否符合需求定义；p0
    • 是否存在与以下内容相同或相近的逻辑，p1
      • 标准库
      • 常用开源库
      • 团队内其他项目中的库
      • 本项目已有的实现
    • 其他不涉及正确性的细节：
      • 公开项(类型、量、方法等)的命名；p1
      • 可见性划分；p2
      • 算法和实现细节；p2
      • 未包含在代码格式化和风格规范内的最佳实践；p2
  • 修复
    • 是否完整覆盖了异常场景；p0
    • 是否引入了其他明显可见的缺陷；p1
  • 重构、优化
    • 是否能通过原始逻辑测试；p0
    • 是否引入了逻辑变化；p1
    • 必要性；p2
  • 测试用例：
    • 存在与否是否符合团队规范和标准；p0
    • 用例的覆盖是否完整；p1
    • 随机源是否合理；p1

文档

对于 markdown 及其他可以以渲染结果查看的文档格式，应当通过 View file 的方式进行审阅，避免直接阅读源文件。

  • 关键内容的准确性：
    • 关键性描述；p0
    • 链接；p0
    • 重要的格式如表格等；p0
  • 段落、层级、缩进设置合理；p1
  • 其他不影响正确性的主观判断；p2
    • 拼写错误
    • 措辞准确
    • 语句通顺
    • 其他格式的使用，如：
      • 粗体
      • 斜体
      • 代码框

自动生成的结果

  • 检查自动生成的过程是否被包含在 Checks 中，且针对文件变化进行了检查；p0
  • 不必检查具体的自动生成内容；

附录

事项规范

划分

  • p0 事项必须完整罗列
  • p1 事项应当尽可能罗列，或经罗列后，可以得出某种“原则性”规范
  • p2 事项可以仅罗列少量具有标志性的内容

解决

  • p0 事项必须在本 pr 中解决，解决结果由发起的审阅者确认
  • 在经过团队一致认可的情况下，p0 事项的解决方式也可以降级等同 p1，但优先级或严重程度方面不得降低
  • p1 事项最好在本 pr 中解决，也可以在得到审阅者认可的情况下延后解决，但必须开列新的 issue 并与此 pr 关联
  • p2 事项解决与否由发起者自决，如果选择解决，则解决结果需要得到审阅者的认可

审阅辅助工具使用规范

  • Conversation

    • p0 事项应当由提出的审阅者确认后点 Resolve conversation

    • 其他级别的事项可以由提出者或解决者点

    • 由于代码提交而自动解决的 Conversation，审阅者需要逐一复查

      如果解决结果存疑，应当发起新的 conversation。

  • Viewed 标记：已经完成内容审阅的文件应当进行标记，以便观察审阅进度，并避免干扰。

  • 行内评论(Comment)：

    • 应当通过有疑问的行行首的 + 号开启
    • 审阅者必须以 Start a review / Add a review comment 的方式进行评论，而不应使用 Add single comment

  • Review changes 面板的使用规范

    • 本次审阅中如果产生了评论，则必须统一提交

      如果存在 p0、p1 事项，进行 Request changes 提交，否则进行 Comment 提交

    • 所有事项都完成检查或得到解决的情况下，应当进行 Approve 提交

      此时通常没有评论，审阅者应当在本面板的评论框中显式标注 LGTM 或其他等效的短语

范例

以下为结合事项规范和审阅辅助工具的一个范例：

  1. 发起 conversation

    flowchart TD
	s([发现问题行]) --> review_comment[发起 review comment] --> is_p2{是 p2 事项}
	is_p2 --> |no|review_select_request_changes[选择 Request changes] --> e([Submit review])
	is_p2 --> |yes|review_select_comment[选择 Comment] --> e
    Loading

  2. 解决 conversation

    flowchart TD
	s([开始解决]) --> read_comment(阅读评论内容) --> get_agreed{达成一致}
	get_agreed --> |no| add_comment(追加评论) --> read_comment
	get_agreed --> |yes| submit_changes(提交修改) --> comment_changes(评论已修改) --> changes_agreed{修改符合预期}
	changes_agreed --> |no| add_comment
	changes_agreed --> |yes| resolve_conversation([Resolve conversation])
    Loading