审阅贡献#
原则#
Arrow 是一个基础项目,需要经过数年甚至数十年的演进,同时为数百万用户提供服务。我们相信,在审阅时细致入微,会比宽容并追求快速合并给项目带来更大的回报。
像这样的代码审阅能带来更高质量的代码,更多参与并理解被修改代码的人,以及一个总体上更健康、有更大成长空间并能适应新出现的项目需求。
准则#
元数据#
运用您自己的判断。这些准则并非硬性规定。提交者应在其工作领域拥有足够的专业知识,以便能够根据其任何疑虑调整其方法。
这些准则没有特定的顺序,也无意作为清单使用。
最后,这些准则并非详尽无遗。
范围和完整性#
我们的总体政策是不引入回归或合并需要后续更改才能正常工作的 PR(尽管可以有例外)。合并后进行必要的更改,不仅对贡献者和审阅者来说成本更高,对其他开发者来说也更具成本,因为如果他们遇到合并的 PR 引入的问题,可能会感到困惑。
PR 的哪些更改属于范围之内,哪些更改可能/可以/应该推迟到范围之外并创建后续问题,应由作者和审阅者协作确定。
当贡献一大块功能并希望分段集成时,在决定如何划分更改时,应优先考虑功能内聚性(例如,如果正在贡献文件系统实现,第一个 PR 可以贡献目录元数据操作,第二个 PR 文件读取功能,第三个 PR 文件写入功能)。
公共 API 设计#
公共 API 应引导用户使用最理想的结构。换句话说,如果有一种“最佳”的方法来做某事,它也应该理想地是最容易发现的,并且输入时最简洁的。例如,安全 API 应该比在无效输入时可能崩溃或悄然产生错误结果的不安全 API 更突出。
公共 API 应理想地倾向于产生可读的代码。一个例子是当预计会随着时间添加多个选项时:最好尝试逻辑地组织选项,而不是将它们全部并列在一个函数的签名中(例如,参见 C++ 和 Python 中的 CSV 读取 API)。
命名很重要。尝试问问自己,在不阅读 API 文档的情况下,调用新 API 的代码是否可理解。应避免模糊的命名;不准确的命名更糟,因为它可能误导读者并导致有 bug 的用户代码。
注意术语。每个项目都(明确或默认地)设定了如何命名重要概念的约定;偏离这些约定会增加贡献者和项目用户的心智负担。相反,将一个众所周知的术语用于不同于寻常的目的也会增加心智负担,使开发人员的生活更加困难。
如果您不确定某个 API 是否适合当前任务,建议将其标记为实验性,这样用户就知道它可能会随时间变化,而贡献者也就不那么担心带来破坏性改进了。然而,实验性 API 不应作为规避基本 API 设计原则的借口。
健壮性#
Arrow 是一套开源库,将在各种各样的上下文中(包括在 Jupyter 解释器提示符下处理故意的伪造数据)使用。如果您正在编写公共 API,请确保它在不寻常(但有效)的输入下不会崩溃或产生未定义行为。
当实现非平凡算法时,防御性编码对于捕获潜在问题很有用(例如,如果语言允许,仅用于调试的断言)。
处理可能不受信任数据的 API,例如磁盘上的文件格式,应尽量避免在输入无效或损坏数据时崩溃或产生无声错误。这可能需要大量的精力,超出了常规代码审阅的范围(例如设置模糊测试),但仍可在代码审阅阶段提出基本检查。
当调用外部 API,尤其是系统函数或处理输入/输出的 API 时,务必检查错误并传播它们(如果语言不自动传播错误,例如 C++)。
性能#
考虑性能,但不要过分纠结。如果输入大小可能“很大”(“大”的含义取决于上下文:请根据您的专业知识来决定!),算法复杂度很重要。对性能敏感的功能进行 20% 或更多性能提升的微优化也可能有用;较小的微优化可能不值得投入时间,尤其是如果它们导致代码更复杂的话。
如果性能很重要,请进行测量。不要满足于猜测和直觉(这可能基于对编译器或硬件的不正确假设)。
另请参阅
除非确实很重要,否则尽量避免通过某种方式编写代码来试图欺骗编译器/解释器/运行时。这些技巧通常脆弱且依赖于可能过时的平台细节,并且它们会使代码更难维护(一个常见的问题,可能会阻碍贡献者,即“对于我的更改想要删除的这个奇怪的 hack,我该怎么办?”)。
避免粗糙的边缘或退化行为(例如,当大小估算不准确时内存暴涨)可能比试图少量改进常见情况更重要。
文档#
这些准则应理想地适用于散文文档和代码内文档字符串。
寻找模糊/信息不足的措辞。例如,“如果…则是一个错误”不如“如果…则会引发错误”或“如果…则行为未定义”信息丰富(前者没有告诉读者在这种错误情况下实际会发生什么)。
在审阅文档更改(或一般的散文片段)时,请注意拼写、语法、表达和简洁。清晰的沟通对于与来自不同背景的人进行有效协作至关重要,并有助于改善文档。
有些贡献者并非以英语为母语(也许您也不是)。建议在需要时帮助他们和/或寻求外部帮助。
交叉链接增加了文档的整体价值。Sphinx 尤其具有出色的交叉链接能力(包括主题引用、词汇表术语、API 引用),因此务必加以利用!
测试#
添加 API 时,所有正常情况都应有测试用例。函数是否允许空值?那么应该测试空值(当然,还要测试非空值)。函数是否允许不同的输入类型?等等。
如果某个功能方面很精细(无论是根据定义还是作为实现细节),则应进行测试。
应处理极端情况,尤其是在 C++ 等低级实现语言中。示例:空数组、零块数组、仅包含空值的数组等。
压力测试可能很有用,例如,如果正在添加非平凡的并行化,可以发现同步错误,或者对照缓慢而直接的参考实现验证计算论证。
然而,一个缓解的担忧是运行测试套件的整体成本。持续集成(CI)运行时可能异常漫长,我们应该警惕过度增加它们。有时值得微调测试参数,以平衡测试的有用性与运行它们的成本(尤其是在涉及压力测试的情况下,因为它们往往意味着在大型数据集上执行)。
标签#
在审阅 PR 时,我们应该尝试确定相应的问题是否需要标记以下一个或两个问题标签
关键修复:此更改修复了以下任一问题:(a) 安全漏洞;(b) 导致生成不正确或无效数据的错误;或 (c) 导致崩溃的错误(同时遵守 API 契约)。此标签旨在标记修复可能在用户不知情的情况下影响用户的问题。因此,修复导致错误的错误不计入其中,因为这些错误通常很明显。导致崩溃的错误被认为是关键的,因为它们是拒绝服务攻击的可能载体。
破坏性更改:此更改破坏了公共 API 的向后兼容性。对于 C++ 中的更改,这不包括仅破坏 ABI 兼容性的更改,除了我们确实保证 ABI 兼容性的少数地方(例如 C 数据接口)。实验性 API 并非不受此限制;它们只是更可能与此标签相关联。
破坏性更改和关键修复是分开的:破坏性更改修改 API 契约,而关键修复使实现与现有 API 契约保持一致。例如,修复导致 Parquet 读取器跳过包含数字 42 的行的错误是关键修复,而不是破坏性更改,因为跳行不是合理用户会依赖的行为。
这些标签用于发布中,以突出显示用户在考虑升级库版本时应注意的更改。破坏性更改有助于识别用户可能希望等待升级直到有时间调整其代码的原因,而关键修复则强调了不升级的风险。
此外,我们使用以下标签表示优先级
优先级:阻塞:表示更改必须在下一次发布之前合并。这包括修复测试或打包失败,这些失败将阻止发布成功进行最终打包或验证。
优先级:关键:表示高优先级问题。这是标记为“关键修复”的问题的超集,因为它还包含对导致错误和崩溃的某些问题的修复。
协作者#
协作者角色允许用户获得分类角色,以便能够帮助分类问题。该角色允许用户标记和分配问题。
用户可以申请成为协作者,或者在参与项目一段时间后由其他社区成员提议。协作可以包括但不限于:创建 PR、回答问题、创建问题、审阅 PR 等等。
要提议某人成为协作者,您必须创建一个 PR,将该用户添加到 .asf.yaml 中的协作者列表。提交者可以审阅该用户过去的协作并批准。
长期不活跃的协作者可以从协作者列表中删除。
社会方面#
审阅是贡献者和审阅者之间的交流。避免让问题或评论长时间未得到答复(“长时间”当然非常主观,但两周可以是一个合理的启发式)。如果您近期无法分配时间,请明确说明。如果您没有问题的答案,请明确说明。说“我暂时没时间,但我稍后会回来,如果我好像忘了,请随时提醒我”或“抱歉,我对此不熟悉”总是比什么都不说,让对方疑惑要好。
如果您认识有能力在某个阻碍性问题上提供帮助的人,并且过去的经验表明他们可能愿意这样做,请随时将他们添加到讨论中(例如,通过轻轻地 ping 他们的 GitHub 账号)。
如果贡献者停止提供反馈或更新他们的 PR,也许他们不再感兴趣,但也可能他们被某个问题困住了,感觉无法进一步推动他们的贡献。不要犹豫,提问(“我看到这个 PR 最近没有更新,你是不是被什么困住了?你需要帮助吗?”)。
如果贡献确实值得,而贡献者没有取得任何进展,也可以主动接手。然而,出于礼貌,最好先征求贡献者的同意。
有些贡献者只是为了解决一个特定问题而寻求快速修复,不希望投入太多时间。而另一些人则渴望学习和改进他们的贡献,使其符合项目标准。后一种贡献者尤其有价值,因为他们可能会成为项目的长期贡献者甚至提交者。
当指出一个问题时,有些贡献者可能会回应“我稍后会修复,我们可以先合并吗?”不幸的是,修复是否真的会在后续的 PR 中很快贡献出来,很难预测或强制执行。如果贡献者之前已经证明他们是可靠的,那么按照建议进行可能是可以接受的。否则,最好拒绝该建议。
如果 PR 除了琐碎或无争议的问题之外,基本上已经准备好合并,审阅者可以决定自己将更改推送到 PR,而不是要求贡献者进行更改。
理想情况下,贡献代码应该是一个有益的过程。当然,它不会总是如此,但我们应该努力减少贡献者的挫败感,同时牢记上述问题。
与任何交流一样,代码审阅也受 Apache 行为准则的约束。这适用于审阅者和贡献者。