Python之美
首发于Python之美
保证代码质量二三事

保证代码质量二三事

之前有同学想了解豆瓣的工程管理、CodeReview,代码规范。我先铺垫一下 @段念@清风 2位老师的分享:

1. 前豆瓣工程副总裁段念谈豆瓣的研发管理

2. 文化建设是个“系统工程”

3. 工程师文化中的工具“情结”

4. 豆瓣 CODE 两年历程回顾:git 不是万能的,没有 review 是万万不能的

5. 清风在 C2D2 的分享Slide

说句场外话,就是因为看了清风老师在C2D2的分享,我才决定来豆瓣的。

在这里聊一些我在豆瓣的一些实践和个人理解。

团队要有自己的规范

开发者提交了PR其实就是潜意识的已经认为自己的代码写的还不错,完成了工作。评审者提意见的表达方式就很重要,弄不好就是矛盾。这可以让很多人本身并不喜欢或者被动的做评审和被评审。

实现同一功能,开发者可以选择多种方式,这就是编程的乐趣。但是问题是没有那么多事情是如「太阳东升西落」这样的客观事实,大部分是很主观的,我说这样实现没有问题,你觉得那样可能会更好,谁也说服不了谁。

「不以规矩,不能成方圆」。 团队要有自己的一些规范和要求,大家习惯了就会遵守,也就不会有那么多的不良情绪。而且这些是根据经验不断改进产生的,也会帮助新人少走弯路。

我们对撰写Pull Request、提供反馈、针对反馈的回应、合并代码等方面都做了比较密集的建议,这个和一些开源项目对贡献代码有一些具体要求是一样的。最近我还引入了Github的PR模板ISSUE模板

提PR的时候会要求你针对情况填写:

What does this PR do?

Which issues does this PR fix or reference?

Trello or Product requirement document path

How can we reproduce the issue?

提ISSUE的时候会要求你针对情况填写:

What's the problem (or question)?

What should have happened?

Do you have an idea for a solution?

How can we reproduce the issue?

有人可能会说,闲的吧? 你确定你那坨代码4个月后你还能讲清楚它是干什么的? 你确定新人维护你的代码时候不会骂你?

事实上,我个人对commit message也觉得应该加一些要求,但是我们目前没有做到。我有时候看着版本库的提交历史很心痛。

除此之外,我们对代码规范也是有要求的,比如:

1. python 代码(包含 mako 模板的 python 代码部分)使用引号时请使用「单引号」

2. Mako 模板变量左右都需要有空格

3. python代码的docstring首行不直接换行

4. 相对引用(relative import) 和绝对引用(absolute import)的具体操作规范

5. SQL编写规范

我们甚至基于业务有一系列的SEPs(Subject Enhancement Proposals)。

有了规范还要坚持。之前发生过一次非常大的对于规范要求的讨论,一个其他组的成员忽视代码规范要求,来提PR被拒绝,继而多个并不在意代码规范的同学来参与。在这里感谢 @VeryCB,通过我们的坚持,之后再也没有发生过不遵守我们团队规范要求的事情。

个人习惯需要向「团队习惯」或者「项目维护者」的习惯做出妥协,在给开源项目贡献过代码的人都应该知道。在团队达成一致的过程中,针对细节的讨论是不可避免的,但是应该鼓励这种讨论。我一直和别人说,在团队按照大家都接受的方式,但是并不要求你认可,如果自己写项目你还是应该遵从内心,其实我有好多时候并不觉得别人的方案是最好的,但是还是认可的,这就够了。如果大家都是一个模子,程序员这个职业也太无聊了。最后再强调,注意方式方法。

有些PEP8没有做要求的规范,也曾经进行激烈讨论。比如「带冒号的二元表达式左右空格的定义」。就是有一天,我想要改进这个PEP8中的空白和其他的1个内容,发了个PR。然后我们组是这样讨论的:

A说: 

https://www.python.org/dev/peps/pep-0008/#pet-peeves

其实 PEP8 对这里没有做非常死的限制,事实上
ham[1:9], ham[1:9:3], ham[:9:3], ham[1::3], ham[1:9:]
ham[lower:upper], ham[lower:upper:], ham[lower::step]
ham[lower+offset : upper+offset]
ham[: upper_fn(x) : step_fn(x)], ham[:: step_fn(x)]
ham[lower + offset : upper + offset]

这些写法都是 PEP8 推荐的

B说:

pep8 +1

按照上面的例子  list[start : start+limit]  或者  list[start : start + limit]  都是比  list[start:start + limit]  要好点

我说:

闲来无事 刚才翻了了我本地的一些项目(先排除不care代码规范的项目). 还看了 sentry, httpie, flake8, twine, warehouse. werkzeug, flask_admin等除了没有这样的用法机会. 就没有其他表达这样用法的方式. 

我检索的代码的方式是  grep ":" ./ -rn|grep "\["|grep "\]"|grep "+"  

pip也是赞同我的  —— PS: pypa这个组织的人大多是python核心开发者, 以及社区重要业务维护者

https://github.com/pypa/pip/blob/develop/tasks/generate.py#L99

但是我看到一个更特别点的用法:

https://github.com/getsentry/sentry/blob/master/src/sentry/templatetags/sentry_helpers.py#L70

[i:(i + break_after)]

C说:

pep8 里说把这里的冒号当作一个二元操作符,两边的算其参数:
list[start:start + limit]

这样的写法看上去要表达的是 start 和 start 的关系更紧密,  :   优先级更高,但是实际上  +  的优先级更高,这也是你列出的最后一种写法为什么要加上括号的原因,这样确实清晰很多,但是每次都打括号有些冗长。为什么不用我提出的那种:
list[start : start+limit]

这样更为清晰的表达了操作符的优先级和各个参数之间的亲密关系,并且也是在 pep8 给的 examples 里的:
ham[lower+offset : upper+offset]

我说:

ham[lower+offset : upper+offset]   但是严格意义不是   ham[lower: upper+offset]   这个的格式.

其次我看  list[start:start + limit]  很舒畅啊 ,   list[start : start+limit]  略怪. 略怪的原因在于运算符直接没有加空格.

 @subject  投票时间到了. 

* 我倾向尊重python标准库以及开源项目的风格, 坚持  list[start:start + limit]  
*  @C    list[start : start+limit]  
编程风格也不一定一定就是统一的一个风格, 要不然这个地方就不做要求,  大家按自己的习惯?

A说:

看完大家的讨论我倾向于这个地方不做规范,按个人风格来写+1

B说:

:lgtm:

结束。 你以为故事就这样结束了么?

有一天,一个不归我们组维护的项目的一个PR莫名的@了我,修改中有如下一段句:

- group_chats = group_chats[start:start+count]
+ group_chats = group_chats[start:start + count]

另外一个同学质疑这是否符合pep8,我又顺便安利了一下:

安利成功!

一般遇到有争议的点,我一般有如下惯例:

1. 看Python标准库中有没有对应的证据

2. 看优秀的、流行的第三方库中有没有对应的证据

3. 看厂内有没有对应的先例

使用合适的工具

有句话叫「工欲善其事,必先利其器」,我们来看看工作中我们用到了那些工具。

1. pre-commit/pre-commit。 持续集成是现在互联网公司必备的一个环节,豆瓣一个专门用来跑CI的服务器集群,你提交PR的时候就会触发,代码能不能被合并都靠它的结果。如果你提交了一个本来就不符合代码规范之类的PR除了可能被吐槽外,还占用了持续集成服务器的资源,尤其是项目比较大的时候,跑一次就要10几分钟,那么这段时间是浪费的。Git支持多种hook(钩子),在你commit或者push代码的时候就可以触发本地的检查,这样本地就可以预先检查出一些问题了。BTW, pre-push功能是我加的。

2. dongweiming/gandalf。在做代码评审时,无论你多么注意方式方法都还是会让对方不咋愉快的。我把代码质量检查作为持续集成的一部分,这样CI挂了,你就看到了。而且我们现在感觉有点默契的潜规则:如果你的CI都没有过,一般都是没人理的,直到你修改的让CI通过。我觉得这些场景人总是像啄木鸟一样出来不断的「啄啄啄」,累还招人烦在,怎么办呢?让程序自动的来发这些检查意见,看下效果:

这样就不用人工的去订正了,解放了生产力。

3. facebook/mention-bot。现在这个工具已经被我推广到整个豆瓣。有时候一个人发了一个PR,他并不知道该提及(@)谁(我一般是看最近活跃的开发者)。有时候其实会@错,别人又会@其他人... 等到真的维护者来时间已经过去很久了。这个工具就是通过算法分辨你在这次的修改谁会更有兴趣继而决定提及谁。用了一年时间,感觉还是蛮准的。缺点是有时候有点烦,因为可能在某个时间上你已经不维护它了,但是由于最近的一些修改是你而被提及。

4. AST。Pylint、PyChecker和PyFlakes都是基于它做的静态检查。但事实上代码规范检查只是一部分内容,其实任何可度量的业务上的东西也都可以检查。我举几个例子:代码中对应的位置有没有加缓存?缓存有没有被正确删掉?用法有没有遵守惯例?SQL拼的有没有安全问题等等。

无耻的广告:《Python Web开发实战》上市了!

欢迎关注本人的微信公众号获取更多Python相关的内容(也可以直接搜索「Python之美」):

编辑于 2017-04-17

文章被以下专栏收录