|
|
|
# 代码审核
|
|
|
|
Google的最佳实践介绍
|
|
|
|
|
|
|
|
## 术语
|
|
|
|
- CL: changelist
|
|
|
|
- LGTM: Looks Good to Me
|
|
|
|
|
|
|
|
## 目的
|
|
|
|
- 为未来的工程师阅读源码考虑
|
|
|
|
- 强迫工程师写出其他工程师能看懂的代码
|
|
|
|
- 不止一个人可以熟悉代码，增加知识在公司内部停留的机会
|
|
|
|
- 如果找到bug很好，但代码审查的主要目的是提高代码的可理解性
|
|
|
|
|
|
|
|
## 好处
|
|
|
|
- 风格、设计的一致性
|
|
|
|
- 确保有充足的测试
|
|
|
|
- 提高安全性 (不能在没有监督的情况下随意提交代码)
|
|
|
|
|
|
|
|
## 紧急情况
|
|
|
|
- 紧急情况下，代码审查需要尽量快
|
|
|
|
- 是什么
|
|
|
|
- 允许一个重要的发布继续而非回滚
|
|
|
|
- 修复一个严重影响用户生产的错误
|
|
|
|
- 处理紧迫的法律问题
|
|
|
|
- 关闭重大的安全漏洞
|
|
|
|
- 不是什么
|
|
|
|
- 希望本周发布而非下周
|
|
|
|
- 除非合作伙伴协议有hard deadline
|
|
|
|
- 开发为一个特性工作了很久，想得到CL
|
|
|
|
- reviewer在异地
|
|
|
|
- 在周五前得到CL
|
|
|
|
- soft deadline
|
|
|
|
- 回滚CL导致的测试失败或生成中断
|
|
|
|
|
|
|
|
# reviewer怎么做
|
|
|
|
## 目的
|
|
|
|
保证代码库的整体代码健康状况随着时间的推移不断改进
|
|
|
|
|
|
|
|
## 权衡
|
|
|
|
- developer取得进展
|
|
|
|
- reviewer确保每个CL的质量
|
|
|
|
- reviewer希望代码库保持一致、可维护，对评审的代码有所有权和责任
|
|
|
|
|
|
|
|
## 准则
|
|
|
|
- 最重要的准则
|
|
|
|
- 即使CL不完美，reviewer也应该支持批准，因为它肯定会提高代码库的整体代码健康状况
|
|
|
|
<!-- - 局限性
|
|
|
|
- 即使代码设计很好，但CL中有reviewer不希望在系统中使用的特性 -->
|
|
|
|
- 没有完美的代码，只有更好的
|
|
|
|
- reviewer不应该要求developer改进CL的每一个小部分
|
|
|
|
- reviewer权衡
|
|
|
|
- reviewer建议的改变的重要性
|
|
|
|
- developer取得进展的需要
|
|
|
|
- 不应该因为代码不完美而推迟数天
|
|
|
|
- 如果不重要，reviewer可以用`Nit:`作为前缀，让developer知道这可以忽略
|
|
|
|
|
|
|
|
## 关注
|
|
|
|
- 设计
|
|
|
|
- 功能性
|
|
|
|
- 复杂性
|
|
|
|
- 行
|
|
|
|
- 函数
|
|
|
|
- 类
|
|
|
|
- 过度设计
|
|
|
|
- 测试
|
|
|
|
<!-- - 单元测试
|
|
|
|
- 集成测试
|
|
|
|
- 端到端测试
|
|
|
|
- 测试也是需要维护的代码 -->
|
|
|
|
- 命名
|
|
|
|
- 注释
|
|
|
|
- [风格](https://google.github.io/styleguide/pyguide.html)
|
|
|
|
- 文档
|
|
|
|
- 每行
|
|
|
|
- 假定类、函数、代码块中的内容没有问题
|
|
|
|
- 确保理解所有代码在做什么
|
|
|
|
- Good Things
|
|
|
|
- 代码审查通常关注错误，也应该对良好的实践提供鼓励和赞赏
|
|
|
|
- reviewer的礼貌
|
|
|
|
|
|
|
|
## 速度
|
|
|
|
- 代码审查缓慢时
|
|
|
|
- 团队速度变慢
|
|
|
|
- developer抗议代码审查
|
|
|
|
- 代码健康影响
|
|
|
|
- 如果不是在专注的任务中，在代码审查到来后立即做
|
|
|
|
- 代码审查请求的最长时间是一个工作日
|
|
|
|
- 一个CL在一个工作日内多轮审查 (如果需要)
|
|
|
|
|
|
|
|
## 编写代码评审注释
|
|
|
|
- 摘要
|
|
|
|
- [善待他人](https://chromium.googlesource.com/chromium/src/+/refs/heads/main/docs/cr_respect.md)
|
|
|
|
- 解释理由
|
|
|
|
- 平衡 给出明确的方向 与 单单指出问题让developer决定
|
|
|
|
- 鼓励简化或添加注释 而非解释复杂性
|
|
|
|
|
|
|
|
# developer怎么做
|
|
|
|
## 好的CL描述
|
|
|
|
- 第一行 简短命令
|
|
|
|
- 空行
|
|
|
|
|
|
|
|
## 错误示例
|
|
|
|
- Fix build
|
|
|
|
- Add patch
|
|
|
|
- Moving code from A to B
|
|
|
|
- Phase 1
|
|
|
|
- Add convenience functions
|
|
|
|
- kill weird URLs
|
|
|
|
|
|
|
|
## 尽量用小的CL
|
|
|
|
- 好处
|
|
|
|
- review时
|
|
|
|
- 快
|
|
|
|
- 彻底
|
|
|
|
- 错误少
|
|
|
|
- 若被拒绝，成本小
|
|
|
|
- 容易merge
|
|
|
|
- 容易设计好
|
|
|
|
- 减少review阻碍
|
|
|
|
- 回滚简单
|
|
|
|
- 多小
|
|
|
|
- 一件事情
|
|
|
|
- 一个特性的一部分
|
|
|
|
- 包括测试
|
|
|
|
|
|
|
|
## 大的CL什么时候可以
|
|
|
|
|
|
|
|
|
|
|
|
# Python风格
|
|
|
|
- [PEP8](https://www.python.org/dev/peps/pep-0008/)
|
|
|
|
- [PEP8 中文版](https://www.cnblogs.com/bymo/p/9567140.html)
|
|
|
|
|
|
|
|
|
|
|
|
# 来源
|
|
|
|
- [Google Engineering Practices Documentation](https://github.com/google/eng-practices)
|
|
|
|
- [Modern Code Review: A Case Study at Google](https://sback.it/publications/icse2018seip.pdf)
|
|
|
|
|
|
|
|
|
|
|
|
# 其他
|
|
|
|
```shell
|
|
|
|
cd xx
|
|
|
|
pyreverse --project Pyreverse . --ignore test --output png # 工程结构图
|
|
|
|
python -m cProfile -o tmp.pstats tmp.py # 性能分析
|
|
|
|
gprof2dot -f pstats tmp.pstats | dot -T png -o tmp.png # 性能分析可视化
|
|
|
|
```
|
|
|
|
|
|
|
|
<!-- # 现代代码审查
|
|
|
|
- 1976 Fagan inspeciton
|
|
|
|
- 发现缺陷
|
|
|
|
- 麻烦
|
|
|
|
- 耗时
|
|
|
|
- 同步性差
|
|
|
|
- 现代代码审查
|
|
|
|
- 同行代码审查
|
|
|
|
- 非正式的
|
|
|
|
- 基于工具的
|
|
|
|
- 异步
|
|
|
|
- 专注于变更代码 -->
|
|
|
|
|
|
|
|
<!-- # [Modern Code Review: A Case Study at Google](https://sback.it/publications/icse2018seip.pdf)
|
|
|
|
- 同行代码审查 peer code review
|
|
|
|
- 1976 Fagan检查法 Fagan inspection
|
|
|
|
- 发现缺陷 defect finding
|
|
|
|
- 麻烦 cumbersome
|
|
|
|
- 耗时 time-consuming
|
|
|
|
- 同步性 synchronous
|
|
|
|
- 现代代码审查
|
|
|
|
- 非正式的
|
|
|
|
- 基于工具的
|
|
|
|
- 异步
|
|
|
|
- 专注于变更代码 --> |
