这是今年coding生涯里最开心的一件事：给最爱用的框架贡献了几行代码，虽然微不足道，但是非常有幸福感，也是特意记下来，把喜悦分享给有缘人~

缘起

一直以来想要突破花瓣和Pinterest对收图的限制，所以最近几天一直在折腾自己部署一个私有的花瓣网类似物，发现了一个叫Pinry的项目，但是作者也比较久没有更新了，feature上不是很完善，所以这几天一直在给这个项目发Pull Request.

在做PR的过程中，发现在采集图片的的过程中，保存WebP格式的文件会出错，而且错误来自django框架而不是 Pinry 项目本身，Django 的 ImageField 在保存图片的过程中，会尝试自动探测图片的尺寸。

为了提升效率，Django会尝试将图片文件的前面部分数据喂到 Pillow 里然后尝试解析出文件的元数据。

因为大部分图片源文件只要读取头部的部分数据就能获取到信息，所以这个方法通常都能工作得非常好，但是对于少部分图片，header 的 size 非常大，或者是分chunk的图片，或者流式的数据，需要读取很多数据才能获得完整的meta信息。

Django 的 ImageField 其实处理了这类问题，但是在捕获错误的时候，有一个 edge-case 没考虑到，就是读取WebP文件的时候，Pillow 会产生一个 RuntimeError ，这个未捕获的错误会导致Django产生内部错误，具体请参考：#29705 RuntimeError while saving webp file to ImageField

流程

提交Ticket前的准备

第一次给Django提交Ticket，首先跑去阅读了，啥都不懂，不过根据之前的经验，大概要考虑以下几个问题：

• 怎么创建Ticket（Issue），怎么描述问题
• 创建一个脚本或者可用的案例，来稳定的复现Bug
• 创建一个Patch，并提交到相应的项目（Django使用Github跟踪和Review）

怎么创建Ticket

赶紧去阅读了一下Django提供的文档： 参见 Contribute to Django，有几个要点：

• Reporting bugs and <strike>requesting features</strike>（因为是提交Bug，所以忽略后半）

  • 要点主要是描述Bug发现的 软件硬件环境, 复现方法， 样例脚本（可选），Bug 出现的 branch 可以在 Trac 系统内选择，要注意的是安全问题需要单独到另一个地方提交。
  • Django Trac 提供的选项非常多，基本上涵盖了整个bug修复流程中的所有中间过程和对bug提出者，修复者的要求。
  • 如果要自己修复，把bug assigin给自己即可

我创建了一个项目用来复现这个bug，并集成了travis-ci用来线上执行这个复现测试

• https://github.com/winkidney/django-webp-image-issue-case
• https://travis-ci.org/winkidney/django-webp-image-issue-case

实际上这个东西可以没有，可以只是简单描述复现条件和复现用的脚本，直接写到Issue里就好。

创建Patch

因为之前没给Django贡献过代码，所以赶紧去看了一下 Django Github Pull Request, 从这里就能看到他们的Pull Request标题是什么格式的，这个格式关联到他们的 CI ， 所以非常重要，可以用来自动关闭 Ticket 。

• 关于DjangoProject的 编码风格，可以参考先前提到的 Contribution Guide
• 代码写好后，要本地跑一遍单元测试，这种Bug需要增加一个回归测试，用于测试以后的更改是否会导致这个Bug再次出现。

我最后的 Pull Request

Review 和 Merge

Django社区的效率非常高，真的非常惊喜，当天就完成了Review和Merge到Master的流程。

中间也得到了很多帮助和修改意见，详情可以参看 Pull Request。

结语

感慨Django社区的效率，第一次给一个非常活跃的项目贡献代码，也是达成了新成就，虽然只是个小Bug，不过非常开心~
希望这个流水账能帮到想要给项目贡献代码的朋友~

PS: 最近也在积极给 Pinry 贡献代码，希望能有更多的给 创作者 们使用的项目，能给 设计师/画师/写作者 们带来更多方便和自由和更高的生产力，也是我今后最想做的事情：）

其他创作者相关的工具：

• PickTrue 图册下载器，最近新弄的项目，用来给 画师/设计师 们构建 Visual Library 目前支持Artstation, Pixiv，花瓣网。
• Pinry - 个人版的花瓣（Pinterest） 可以不受限制（比如NFSW，版权）的限制收图，当然记得尊重版权，收图可以，不要到处传播哦。
• TabArtstation - Chrome插件 ， 在Chrome新标签中查看Artstation每日更新，收图必备，适合没时间去专门刷Artstation的同学
• huaban-exporter - 花瓣导出器， 很早之前做的花瓣备份工具，会连同花瓣的元数据（比如描述，图片源链接）一起下载到本地，如果没有元数据需求，可以直接使用PickTrue

引子

有朋友问到code-review应该怎么做，有没有什么规范或者最佳实践呢？
很早之前就想聊一聊这个话题，一直没有时间，这次专门来讨论一下 code review 的动机和选型的话题。
讲过 code review 的优秀文章已经很多了，我希望从“找出事物背后驱动它的动因”的方式来分析一下为什么要 code review，该怎么 code review。

基础

• 对git工具，git branch，merge conflict有概念
• 对 code review 的概念有基本的认识

如果对Git不是很熟悉的同学，可以参考 Git Tutorial - Git 入门教程
关于 code review : 维基百科

进行 code review 的动机

所有范式和解决方案存在的理由都是为了“解决问题”。

code review 能干什么

review 的过程本质上是一个沟通的过程，通过一来一回的 comment 和 反馈，可以

• 增强代码的可读性
• 增强可维护性（可读性提高的副产品）
• 知识共享和经验传递
• 发现一些潜在的bug

如果将代码质量分为 1）可编译，2）可运行，3）可测试，4）可读，5）可维护，6）可重用
在 team-work 中 review 之前的代码，通过测试的情况下只能达到3的水平，review 之后的代码则可以达到4甚至更高的水平。

code review 不能干的事情

• 彻底消除bug
• 缩短工期
• 统一代码风格

code review 的组成部分

我们先从 code review 的组成部分开始，可以从它的组成部分，也可以逆推出 code review 适用的场景，它解决的问题，并且回答“我们是不是应该进行 code review” 这个问题。

code review 通常由以下部分组成

• 需要进行 code review 的项目
• 进行 code review 的人
• 用于进行 code review 的工具链/工作流

需要进行 code review 的项目

什么样的项目需要进行 code review ？

• 项目的重要性：项目是否需要严格的代码审计？对 安全漏洞/逻辑错误 的容忍度有多高？ 要求高的项目需要 code review 流程来提高质量。
• 项目的规模：项目复杂还是简单？相对独立还是比较混乱？复杂的项目相对来说更加需要 code review 来减少错误。
• 是否协同开发： 项目是否有复数个维护者？如果是一个人维护，则对 code review 的需求会小很多。
• 项目的生命周期：项目是一次交付不需要维护，还是需要一直持续更新维护？需要长期维护的项目对代码质量有更高的要求。
• 开发周期：项目有多少时间可以用来开发？code review需要额外的时间来进行，可能是开发用时的两倍甚至更多；如果时间不够，可能 人力/时间 资源没法让你进行 code review -- 每一次工程都是质量向deadline妥协的结果，不过也请记住，欠的技术债，还的时间越晚，利息就会越高。

如果上述问题都没有，很可能你的项目没有进行 code review 的必要，自己做得开心就好：）

进行 code review 的人

人是 code review 得以进行的必要元素之一，code review 过程的主体是代码编写者，执行者就是 reviewer，负责进行 review 的过程，为 author 提供反馈。
因此 code review 得以进行的基础是2个以上的人一起为同一个项目工作。

code review 的核心是人的共识，共识有两种

• review 前的共识
• 在 review 中希望达成的共识

review前的共识

review 之前的共识，应当是具体的东西（否则无法达成共识），需要事先约定，大家约法三章，之后像宪法一样遵守它。
具体的比如：

• 代码风格(Style Guide)： 这种Guide涉及开发的方方面面，可以用人家现成的风格，改造之后适合自己团队的现状就好。
• 项目约束：比如包的管理方式，相互之间的交互方式

大部分代码风格问题可以交给机器来解决，各种语言的lint工具（比如flake8），甚至lint工具能帮你发现一些逻辑上的错误。
所以，其实需要 review 的 “代码风格问题” 主要是在命名规范上面，比如能不能用拼音，用全称还是缩写，常量要怎么写？诸如此类。

review 中希望达成的共识

这个共识的内容应当是不那么具体的事情，因为不那么具体的事情一定会受到人的主观的影响

• 完成核心目标： 增强代码质量，具体来说，是可读性，可维护性，可重用性等等。
• 完成所选工作流的核心目标：后面对比工具链和工作流的时候回详谈。
• 传递经验：比如新成员接管老系统，老成员可以在 review 中具体问题具体分析，带新成员熟悉项目。

这些东西受到个人主观的影响非常大，所以在 review 中会产生很多矛盾，有一些小Tips可以帮助你解决部分问题。

工具链/工作流

工具链和工作流，是无法分离的整体，工具链直接决定了你能使用的工作流，所以对于工具链的选型，直接影响了你能使用的工作流。

我接触过的两种工作流，一种是基于gerrit + Jira + CI 的工作流，另一种使用 github/gitlab + CI 作为项目管理工具。

• Gerrit + Jira + CI：Gerrit 是 Google 推出的代码review工具，和Jira有良好的集成，Gerrit工作流中，review的单位是 commit， 特色是可以管理 不同的 commit 之间的依赖关系OpenStack的GerritWorkflow
• Github + CI / Gitlab ：Github 和 Gitlab 使用更加“现代”的工作流， 参考git-workflow

具体的工作流实施方式，可以参考工作流后面附带的链接，我主要想聊聊用Gerrit来管理多Production的工作方法(感谢老东家SmartX)。

用 Gerrit 工作流管理多Production版本的方法

• Gerrit 工作流中最核心的命令是 rebase， 你需要每次发起review之前，保证自己的代码已经从当前 master 上执行了 rebase，这保证你的代码在被review的时候总是没有冲突的。
• Gerrit 工作流非常强调每个单独的 commit 都需要是能单独合入master并且不会产生副作用的， Topic 是 Gerrit 用来 组织一个Feature的单位，但理想情况下，每个 Gerrit Commit都是是一个可以的独立运行的Feature。
• 管理多分支是 gerrit 工作流最擅长的事情，尤其对于2B类产品，维护多个生产版本是很常见的事情。
• 因为 Gerrit 每个 review 的单位是 commit 同时强调每个 commit 尽量小， 所以 review 起来工作量相对较小。
• 每个 commit 都要求 CI 能跑过。

如图所示，master 分支就是 Gerrit 工作流的开发分支，所有的commit都会被先合入主分支，待验证通过后，被 cherry-pick 到生产分支，对于过老的生产分支，可能只需要 cherry-pick bugfix 而不会将新的feature给 backport 到老分支。

正是因为强调每个commit的独立性和每个 commit 的内容的精简，cherry-pick 才能在 gerrit 工作流中工作良好。

对于 Github 类的 workflow，一个 feature-branch 可能有数十个 commit， 显然没法做到这么细粒度的管理。

用 Github/Gitlab 工作流管理单Production版本

本质上，用github工作流管理多分支多版本发布的区别是不大的，直接参考链接里面阮一峰老师的多版本的图，也是通过cherry-pick把新版本的feature应用到老版本。

区别主要是以下几个：

• cherry-pick 在应对 feature-branch这样有很多commit的时候，难以形成有效的提交记录，界限非常不明确，在稳定发行版中，可能会造成revert的时候的巨大困难。
• feature 难以被以很低的粒度应用到老版本，还是feature-branch的问题，feature-branch包含的commit太多会导致应用困难
• feature-branch review 起来困难比较大，因为代码量可能会很大。
• 优点是每一次从别的 branch 合并到主分支，能容纳非常大的feature提交，对于粒度控制的要求相对较低，降低了设计和编码的成本，以解决问题为目的。
• feature-branch 提交的时候，CI 只需要最后通过即可，所以可能会造成 cherry-pick 某个 commit 后，break掉整个分支。

团队应该根据 code review 的动机进行选型

综上所述，如果你的项目倾向于2C的产品，生产版本始终只有一个，或者最多多个预发行测试版，使用github/gitlab 工作流可能会更加轻松，用起来更加符合人类习惯。

如果你需要同时维护很多稳定发行版本，使用github/gitlab工作流，需要很多额外的规范和工作量，此时使用 gerrit 工作流，能给你带来很多好处，省很多心。

如果你们只是为了相互传递知识，其实任意一种方式都可以，但是出于简单起见，我推荐使用 github/gitlab 的工作流，发PullRequest 然后大家 Approve 就可以了。

code review 容易遇到的常见问题

冲突

代码冲突

代码冲突是每个团队合作都会遇到的问题。
我过去的工作中，一般遵循如下规则

• 每个feature提交者在提交的时候必须保证自己的代码和master没有冲突，并且必须使用rebase来保证fast-ward，避免将主分支merge到feature-branch，这样会产生非常混乱的提交历史，几乎不可读。
• 充分测试，起码要测试到代码冲突的部分，否则难以保证解决冲突的代码是没有问题的。

观念冲突

• 对于风格分歧较大： 保证相互交互的API部分的共识即可，其他部分爱怎么写怎么写，只要能过lint就行
• 实现方式分歧较大：这个必须要达成共识，有必要找第三者 review 并确定最后方案
• 基于以上原因，每个 commit 很小，可以减少实现完了一个巨大的功能然后 review 的时候才发现不能被接受的情况。

小Tips

• 永远记住，code review 是为了和人沟通，就事论事，没有谁优谁劣。保持这种态度，你的 review 进程的推进会容易很多。
• CI 是 code review 中基础的基础，CI 系统的建设是 code review 得以进行的基础，一旦系统有了CI和lint，没有人再有借口开脱自己写出了烂代码。
• 烂代码是阻止不了的，唯一的阻止方法是：不要和这个人维护同一个项目。
• 代码的可读性永远比简洁更加重要，甚至比性能更加重要，因为这个项目是需要继续维护下去的。
• 在修改成本较低的情况下，可以选择更优的实现方案，这种事情可以放在 code review 做，但是不要因为强迫症而强迫要一个人使用一个成本高昂的但是收益很低的方案。
• Typo是可以提出的，因为机器没法检查到所有的Typo。
• KISS，保持代码的简单易懂，对于大部分需求，只要实现最简单的一个函数也许就能解决，不要尝试实现一个复杂的范式或者一个通用度极高的抽象，因为将来需求会变化的，而且不一定和你的抽象一致，所以保持简单吧，将来改起来也容易，不会牵连过多。
• 英文命名方面，多查查词典吧：）
• 没法统一用英文的时候，就用中文写 commit message吧，后来的人会因为能读懂你写的 commit message 而感谢你的（比半吊子英语好太多了）。

题外话：team-work的品味

暂时只讲几个关键词吧

• 看完整个需求文档，包括不是自己做的部分，明确自己的功能设计能不能满足其他部分的需求（说起来容易做起来难）。
• 相互交互的API保持一致，事先就商量好。
• 为别人提供服务，让别人不关心你的实现，调用你的API就好，最好是做好封装而不是只写文档。

说起这个需求，其实是来自于实现一个Gevent内的代码执行器的逻辑。

这个逻辑要求定时运行某个用户的callback，但是必须不能一直执行这个callback，所以要有Timeout，在Timeout到达的时候，自动kill掉用户的Greenlet.

- 阅读剩余部分 -

https://github.com/pallets/werkzeug

文件在 werkzeug/local.py

看这部分源码，主要想搞清楚以下几个问题：

• ThreadLocal解决什么问题
• ThreadLocal如何实现
• ThreadLocal的生命周期管理

ThreadLocal解决什么问题

ThreadLocal是需要拿来和全局变量对比的。

当大家都需要用相同的逻辑，引用相同的变量名/资源来完成自己的逻辑，但又不希望不同的线程直接一个全局引用会对别的线程造成影响，需要使用ThreadLocal来解决这个问题。

ThreadLocal 解决的不是多线程编程资源共享的问题，更多的是在逻辑层面，用来管理一些跟随线程生命周期的上下文数据，让程序逻辑更加容易编写和维护。

所有的代码能公用同一个逻辑，而不会对另外的线程（全局资源）造成影响。

ThreadLocal如何实现

每一个运行中的Thread都会有自己的一个标识符，这个标识符是线程数据结构的一部分。

通过如下步骤，可以实现ThreadLocal

一个进程内全局的Storage，依靠一个Map来存储ThreadID和其对应的数据。
接下来，使用Local()来获取本地变量，实际上是一个查询函数用于获取数据，这个函数帮忙做的事情，就是自动获取Thread ID，从全局的ThreadVariableMap中获取到线程对应的数据集合。
如此一来，每个线程，都可以使用相同的代码逻辑来执行逻辑而不会对全局资源产生影响/依赖。

在Werkzeug中的ThreadLocal大概是这样的结构。

+---------------------------------------------------------+
|                                                         |
|         +----------------------------------+            |
|         |                                  |            |
|         |    LocalStack/LocalsRegistry     |            |
|         |                                  |            |
|         +---------^------------------------+            |
|                   |                                     |
|                   |                                     |
|                   |                                     |
|                   |                                     |
|                   |                                     |
|   +---------------+-------+                             |
|   |                       |                             |
|   |  Local Variables      |                             |
|   |  Marked by Thred-ID   |                             |
|   |  or Greenlet-ID       |                             |
|   |                       |                             |
|   +-----------------------+                             |
|                                                         |
+---------------------------^-----------------------------+
                            |
                            |
                            |
                            |
                            | Identifier(Greenet-ID or
                            | Thread-ID)
                            |
+---------------------------+-----------------------------+
|                                                         |
|                       LocalProxy                        |
|                                                         |
+---------------------------^-----------------------------+
                            |
                            |
                            |
+---------------------------+-----------------------------+
|                                                         |
|                     Application                         |
|                                                         |
+---------------------------------------------------------+

ThreadLocal的生命周期管理

ThreadLocal是的生命周期是跟随Thread的生命周期的。

通常来说，ThreadLocal应该在Thread生命周期结束的时候进行销毁和清理，不然就会造成内存泄漏。

在Werkeug中，ThreadLocal用于WSGIRequest和WSGIResponse，可以看到，ThreadLocal被作为一个Middleware被插入WSGI处理流程中，然后在Response返回执行完毕之后被销毁。

这两天读了一下Python的Condition实现源码，是实现Queue的工具之一，发现是非常朴素的sleep->loop->query模式。源码很少，直接贴出，就不做注释了：）

def wait(self, timeout=None):
        """Wait until notified or until a timeout occurs.
 
        If the calling thread has not acquired the lock when this method is
        called, a RuntimeError is raised.
 
        This method releases the underlying lock, and then blocks until it is
        awakened by a notify() or notifyAll() call for the same condition
        variable in another thread, or until the optional timeout occurs. Once
        awakened or timed out, it re-acquires the lock and returns.
 
        When the timeout argument is present and not None, it should be a
        floating point number specifying a timeout for the operation in seconds
        (or fractions thereof).
 
        When the underlying lock is an RLock, it is not released using its
        release() method, since this may not actually unlock the lock when it
        was acquired multiple times recursively. Instead, an internal interface
        of the RLock class is used, which really unlocks it even when it has
        been recursively acquired several times. Another internal interface is
        then used to restore the recursion level when the lock is reacquired.
 
        """
        if not self._is_owned():
            raise RuntimeError("cannot wait on un-acquired lock")
        waiter = _allocate_lock()
        waiter.acquire()
        self.__waiters.append(waiter)
        saved_state = self._release_save()
        try:    # restore state no matter what (e.g., KeyboardInterrupt)
            if timeout is None:
                waiter.acquire()
                if __debug__:
                    self._note("%s.wait(): got it", self)
            else:
                # Balancing act:  We can't afford a pure busy loop, so we
                # have to sleep; but if we sleep the whole timeout time,
                # we'll be unresponsive.  The scheme here sleeps very
                # little at first, longer as time goes on, but never longer
                # than 20 times per second (or the timeout time remaining).
                endtime = _time() + timeout
                delay = 0.0005 # 500 us -> initial delay of 1 ms
                while True:
                    gotit = waiter.acquire(0)
                    if gotit:
                        break
                    remaining = endtime - _time()
                    if remaining <= 0:
                        break
                    delay = min(delay * 2, remaining, .05)
                    _sleep(delay)
                if not gotit:
                    if __debug__:
                        self._note("%s.wait(%s): timed out", self, timeout)
                    try:
                        self.__waiters.remove(waiter)
                    except ValueError:
                        pass
                else:
                    if __debug__:
                        self._note("%s.wait(%s): got it", self, timeout)
        finally:
            self._acquire_restore(saved_state)