# 在 GitHub 上玩转开源项目的 Code Review


## 一、幕后故事

时光荏苒，岁月如梭…… （🤮太文绉绉了，这不是我的风格）

今天我准备聊聊在 GitHub 上如何玩 Code Review。

突发奇想？心血来潮？不是。

咋回事呢？（对八卦不感兴趣的可以直接跳到下一节，但是我猜你会感兴趣。）

---

首先我是 DevStream 开源社区成员。

在五月份，又有3位活跃的优秀的牛X的 Contributors 正式加入 DevStream 开源社区，正式成为了社区 Member！

(看下面的红框框)

{{< figure src="members.png" title="DevStream Members" >}}

于是乎，加上三月份的4个“老 Member”，DevStream 社区就有7个“社区 Member”了（社区 Member 区分于像我这种在思码逸上班的内部 Member）！

7个疯狂输出的 Members，外加接近20个 Contributors，我和[铁心](https://github.com/IronCore864)两个人基本就只能看着 pr 笑笑，一边表示欣喜，一边表示 review 不过来了，“应接不暇”，废了废了……

也就是说，是时候组织一个 reviewer 组，拉着大家一起玩 Code Review 流程了！

说到 Code Review 流程，流程是啥？规范是啥？规则是啥？技巧是啥？xxxx？我能预想到 reviewers team 这个事情落地之日会有一堆问题砸到我头上。好吧，我需要写一篇文章来聊聊这些事。

## 二、踏上旅途

下面我们开始一次 Code Review 之旅。

### 1. 抢票阶段：认领一个 Review 任务

开始一次 review 之前，首先咱得“认领”一个 review 任务。

怎样算成功认领？如下图，Reviewers 里有你的头像，这时当前 pr 你就是 reviewers 之一，同时可以看到黄色 bar 里的一行字“*This pull request is waiting on your review.*”以及绿色的按钮“**Add your review**”。你可以点击这个“**Add your review**”开始一次 Code Review 之旅。

{{< figure src="reviewer.png" title="Request Reviewer" >}}

“**那么怎么认领呢？**” 可能你还想问我。

这个问题有答案，也没有答案。

因为你是 reviewer 之一，那么你就有权限自己点击 Reviewers 右边的⚙️齿轮按钮，然后指定自己是一个 Reviewer。如果你不是一个“合法”的 reviewer，那么你得先成为 reviewer (If you want)。

### 2. 持票上船：开始 Review 流程

点击 **Add your review** 按钮后，咱就进入到了网页版 Code Review 页面，大致如下：

{{< figure src="reviewpage.png" title="Code Review Page" >}}

这里有很多值得“探索”的特性，比如：左边的“文件树”、文件树上方过滤“commits”的下拉框、右边的“文件过滤”、每个文件右上角的 Viewed 选择框、…… 每发现一个新功能，你的 review 过程就会多简化一分。

关于这个页面，没有比[官方文档](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request)更权威和详细的介绍了，我想我没有理由“搬一次砖”，大家点击[链接](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request)进一步探索吧。

对于简单的修改，或许网页直接查看代码 diff 就足够了，类似这种变更级别的 pr：

{{< figure src="pr1.png" title="Minor Changes" >}}

我们可以轻松判断这个修改是不是“正确”，然后选择进一步的操作，比如 Approve：

{{< figure src="lgtm.png" title="lgtm" >}}

### 3. 下船休整：下载一个 pr 到本地 Review

对于一个不能“一眼看穿”的 pr，比如对方没有附上详细的测试结果等信息来证明他的修改已经“充分测试”（[充分测试的例子](https://github.com/devstream-io/devstream/pull/405)），这时候靠肉眼我们很难判断这个 pr 合入后会怎样，或者不借助 IDE 的能力我们甚至很难看懂一些复杂的修改，这时候就需要下载这个 pr 对应的代码到本地，然后用 IDE 来帮助 review 了。

以 [pr #641](https://github.com/devstream-io/devstream/pull/641)为例，我们需要下载这个 pr，这时只需要执行这样两条命令：

```sh
git fetch upstream pull/641/head:pr641
git checkout pr641
```

效果如下：

{{< figure src="pr641.png" title="pr641" >}}

然后在 IDE 里打开项目，就能看到 pr 对应的代码了：

{{< figure src="ide.png" title="IDE" >}}

“**代码在手，天下我有！**”

这会你可以在本地仔细查看每一处代码细节，可以在本地跑一下 `make buid -j8` 或 `go test ./...` 之类的命令来逐步“打消自己内心的疑虑”。当然，pr 本身会触发 GitHub actions workflows，这些基础的 *build/ut* 流程其实本地不跑也能知道是不是有错误，我们可以直接在页面上看到（每个项目具体的 ci 逻辑不一样）：

{{< figure src="check.png" title="Checks" >}}

到这里，你就基本能够完全看懂一个 pr 对应的所有修改了，是放他过？还是拦下马？他的“命运”掌握在你的手里！

## 三、移花接木：提交一个 commit 到别人的 pr

可能有时候，你需要修改别人的 pr。哥们，我建议你抽根烟冷静一下，再问自己一句：我真的必须去修改他的 pr 吗？这样做会不会被打？

一般情况下，我不建议你去修改别人的 pr，尤其是不能保证你的修改一定正确的情况下。因为你别人的 pr 本身就是容易冒犯到别人的事情，其次万一你改了之后发现还需要别人自己补一个 commit，他会在复杂的 git 操作中骂你一万遍。

什么时候需要去动别人的 pr 呢？举个例子，比如[这个 pr](https://github.com/devstream-io/devstream/pull/589)。

首先这是一个 new contributor 提交的 first pr，所以我不希望他的 first pr 之旅太艰辛。然后这个 feature 其实并不简单，虽然技术上看并不难，但是要做到“不重不漏”就需要对 `dtm` 命令的所有子命令都“了然于胸”才行。显然，这对一个 new contributor 来说要求太高了。所以在他提交了一个 commit 之后，我直接在这个 commit 的基础上又加了一个 commit，完成了剩下的工作，并且在认可他的工作后告知其为什么我要修改这个 pr，并附上了测试结果等。

{{< figure src="pr589.png" title="Change Others pr" >}}

具体怎么操作呢？步骤如下：

1. **修改代码，本地 commit**

前面我们已经下载了一个别人的 pr 到本地，接着自然是继续修改，然后提交 commit（`git add xxx & git commit -s -m "xxx"`），到这里都没啥技术含量，不赘述了。

2. **找到 pr 对应的源分支**

在 pr 页面可以看到具体 pr 是从哪里提交的，比如：

{{< figure src="branch.png" title="Find The pr Source" >}}

我们点进去，就可以找到 fork 项目的地址信息，像这样：

{{< figure src="fork.png" title="Fork" >}}

于是，我们可以这样来加一个 remote：

```sh
git remote add himku git@github.com:himku/devstream.git
```

此时这个 pr 对应的 fork 项目的地址是 `himku`(git@github.com:himku/devstream.git)，对应分支是 `fix-issue-559`，于是我们可以这样将自己新增的 commit(s) 提交到这个 pr 里（本地 commit 后执行）：

```sh
git push himku HEAD:fix-issue-559
```

{{< figure src="push.png" title="New Commit" >}}

是不是很酷？

## 三、乱七八糟的规则：目的是啥？规范是啥？要求是啥？啥是啥？

再聊聊剩下的一些零零碎碎的问题，比如可能你想问 review 的目的啊，规范啊，要求啊，啊啊啊……

### 1. 目的是啥？

可能你会问我为什么要 code review ？我希望你别问。因为我不想总结。（这个问题可以 Google 到一堆答案）

我相信你心中有答案，code review 对应的是一堆的“褒义词”，比如：发现错误、保证质量、互相学习……

你想的都是对的，总之这个事情值得做就对了，不需要去总结为什么。

啥？

你还是想听我谈谈？

谈谈就谈谈。

- **软件质量**：首先大多数的错误可以在 code review 阶段被暴露出来，这些错误留到日后“爆雷”再被修复，代价会大很多，不管是造成的业务负面影响，还是额外付出的修复时间。所以 code review 看似多花了时间，其实整体看是提升交付效率的；
- **代码质量**：严格执行 code review 流程一方面可以互相督促对方：你别随便提交“垃圾”代码上来，会有人 review；一方面假如有“垃圾”代码被提交上来了，可以有人站出来及时制止。完善的 code review 流程可以避免代码可读性日益变差，维护成本日益增大，逐步变成“屎山”；
- **传播经验**：如果我写的代码很漂亮，我希望你来 review，这时候一方面我想“秀”，无需避讳；另外一方面我希望你能够学着点，这是为你好；如果我的代码写的很烂，我希望你来 review，这时候我希望你可以给我指出问题，帮助我提升编码水平，这是为我好；总之互相 review，互相学习，你好我好大家好；

### 2. 规范与要求是啥？

当我们解决了所有“流程”或“技巧”层面的问题后，下一个“非技术性”问题是：**怎样的代码需要“返工”？怎样的代码可以被合入？**

我相信你心中会有这样的疑问。

对于有“错误”的代码，毫无疑问，不能合入，这不在我们的讨论范围。

那么正常运行，没有逻辑错误的代码呢？比如“代码风格有点混乱”，比如“缺乏必要的一些注释”，比如“可读性差”……

我们分三个层次来思考：

- **严格**：我们完全可以指出任何是“问题”的问题，因为一份 WTF 的代码被合入后，所有对 coder 的骂声都包含一句潜台词“reviewer 干啥吃的？”所以很简单，你觉得有问题的地方都可以提出来，包括：
  - “代码太复杂，看起来费力，我觉得可读性不好。”
  - “我看了十分钟还是看不懂，说明可读性不够好。”
  - “这个函数太长了，我鼠标滚了好几下才看完，你应该拆分一下。”
  - “这个函数从名字我看不出来功能，但是我懒得看具体逻辑，为什么没有更多的注释？”
  - “你这个源文件都五百行了，你要不拆分一下？”
  - “这个包的逻辑很关键，你应该加点 UT？”
  - ……

{{< figure src="wtf.png" title="WTFs/Minute" >}}

- **一般**：如果一份代码功能完全正确，可读性也勉强还行，或许没有很好的面向对象来组织，或者注释不太详细，或者存在其他一些小小的不完美。这时候你也可以选择通过，避免太严格的 review 规则把一个 pr 的合入周期拖的太长，一方面影响交付效率，一方面打击开发者信心。很多时候我们可以对自己，或者对核心开发者要求更严格一些；但是对于社区贡献者，往往需要更多的“宽容”与认可。

- **温柔**：如果是一个 new contributor 提交的一个 first pr，这时你可以放下各种能放下的要求，只要这份代码还过得去，就能合入，没有啥比给新人一些信心更重要的。如果 pr 存在一些小问题，你觉得对他来说改起来不会太困难，你可以留言友好的告诉他哪里需要改，怎么改；如果你觉得对他来说进一步做到“完美”有难度，你也可以直接提一个 fix 的 commit 到这个 pr 里，直接帮他修复问题，然后留个言告知他没有考虑到的问题是什么。

## 终点站到了，下船！

今天就聊到这里。

意犹未尽？

再去看看 Google 的 [Code Review Developer Guide](https://google.github.io/eng-practices/review/) 吧！

---

- 你可以收藏我的个人网站 <https://www.danielhu.cn>，阅读更多我写的博客文章；
- 你也可以关注我的个人微信公众号“胡说云原生”，第一时间接收新文章推送；
- 当然，你也可以收藏 DevStream 的博客网站 <https://blog.devstream.io> 或者官网 <https://www.devstream.io>，里面不止有我这种“不严肃”的人发的“主要用来搞笑”的文章，还有 DevStream 团队其他成员发的“严肃讲知识”的“科普文”。