拉取请求审查指南
编辑拉取请求审查指南
编辑对 Kibana 的每一项更改都必须保持高标准，虽然拉取请求的质量责任最终在于作者，但 Kibana 团队成员作为审查者有责任在审查过程中进行验证。
坦率地说，不可能构建一个包含我们在审查拉取请求时可能遇到的所有情况的具体要求列表，因此本文档尝试列出一些常见的显式要求，同时也概述我们在进行任何 PR 审查时应持有的一般理念。
PR 审查不应也不期望以本文档的形式进行，其中最上面的准则始终首先应用，而最下面的准则始终最后应用。整个文档应被视为一个整体，而不是一系列必须按顺序完成的连续待办事项。
虽然审查过程始终由 Elastic 工作人员完成，但这些准则适用于所有拉取请求，无论它们是由社区成员还是 Elastic 工作人员编写的。
目标受众
编辑本文档的目标受众是拉取请求审查者。对于 Kibana 维护者来说，PR 审查是我们对贡献过程唯一完全控制的部分。任何给定拉取请求的作者可能不了解我们对拉取请求的最新期望，他们可能从未阅读过我们的指南。作为审查者，我们有责任指导大家完成此过程，但如果没有一套通用的文档化原则，就很难做到始终如一。
拉取请求作者也可以从阅读本文档中受益，因为它有助于在作者和审查者之间尽早建立一套共同的期望。
快速拒绝
编辑每个拉取请求都不同，在审查任何给定的 PR 之前，审查者应考虑审查 PR 的最佳方法，以便如果最终拒绝更改，则应尽早在过程中完成。
例如，对于包含新 UI 功能的 PR，审查者可能希望尽早进行产品级别的审查。另一方面，也许作者正在提交一个新功能，该功能过去因关键的架构决策而被拒绝，在这种情况下，审查者可能需要在深入研究其他任何内容之前，先关注架构的合理性。
三大要素
编辑我们在所有拉取请求中都希望遵循许多离散的要求和指南，但有三件事特别重要，高于其他一切。
-
我们是否希望产品中包含此更改？
仅仅因为一个人花时间构建了某些东西，并不意味着我们希望该更改存在于产品中，这就是为什么我们鼓励每个人在开始操作之前先打开 issues 描述他们想要做什么。
我们添加的每个功能都会永久性地将我们的一部分注意力从构建和支持其他功能上转移开。我们实际上可以维护的功能数量是有限的，因为它们都需要我们花费一些时间和精力，因此至关重要的是，对项目的增强不仅要对其质量进行审查，还要对其增加的价值、增加的维护负担以及它们与我们项目的长期目标是否密切相关进行审查。
形势也会随着时间的推移而变化，因此重要的是，我们要不断重新评估我们围绕是否希望在项目中进行某些更改而做出的决策。一些更改需要花费大量时间来构建，而且很可能在我们将它们完成时，情况已经发生了足够大的变化，以至于我们不再希望在项目中进行此更改。
-
此更改的架构是否合理？
围绕如何从技术上构建更改做出错误的决策意味着我们立即承担了技术债务，这将不必要地分散我们未来改进项目的精力，因为我们需要维护现有更改、追查错误并最终在未来重新构建该区域。
没有一种通用的方法来评估给定更改在技术上是否合理，因此我们必须依靠我们构建软件的具体知识和经验。当两位经验丰富的开发人员对什么在架构上合理以及什么不合理有不同意见时，这尤其难以做到，因此作者和审查者都需要展现同理心，尽力理解对方，并就前进的道路达成共识。
这里的目标不是找到一个最佳架构，因为它不太可能被限定。任何问题通常都有许多架构合理的解决方案，因此审查者应客观地关注该目标，而不是关注他们自己对如何构建更改的愿景是否“更好”。
-
是否有足够的测试来确保此更改将来不会中断？
如果我们依靠手动测试来捕获错误，我们就无法扩大我们的开发工作并继续有效地改进和维护产品。手动测试是我们最后一道防线，它是确保产品稳定的最无效和最昂贵的方法。这意味着我们对产品所做的每一项更改都需要有足够的测试覆盖率。
这不仅仅是一个关于是否有足够测试文件的问题。测试中的代码本身与产品中的代码一样重要。
我们所有的代码都应具有单元测试来验证其行为，不仅包括“happy path”，还包括边缘情况、错误处理等。当你更改模块的现有 API 时，应该始终至少有一个失败的单元测试，这反过来意味着我们需要验证所有使用该 API 的代码在必要时是否正确处理了更改。对于级别较高的模块，这意味着我们在产品中进行了重大更改，我们需要相应地处理这些更改。
除了广泛的单元测试覆盖率外，PR 还应包括相关的功能和集成测试。在某些情况下，我们可能只是在测试与文件系统、网络、Elasticsearch 等集成的编程接口（例如，服务）。在其他情况下，我们将通过 HTTP 测试 REST API 或将屏幕截图/快照与先前已知的可接受状态进行比较。在最坏的情况下，我们使用 selenium 在 Kibana 的运行实例上进行基于浏览器的功能测试。
增强功能几乎总是会以广泛的单元测试作为基础，以及功能和集成测试。错误修复应始终包括回归测试，以确保将来不会再次出现相同的错误。
产品级别审查
编辑审查者不仅仅评估代码本身，他们还在评估产品中面向用户的更改的质量。这通常意味着他们需要在本地检出分支并“体验”它。除了“我们是否希望产品中包含此更改”的细节之外，审查者还应查找错误，并评估该功能在实施时的可访问性和实用性。应特别注意错误场景和边缘情况，以确保它们在产品中都得到妥善处理。
一致性、样式、可读性
编辑拥有相对一致的代码库是我们构建可持续项目的重要组成部分。在任何给定时间都有数十名活跃的贡献者的情况下，我们依靠自动化来帮助确保一致性 - 我们通过 CI 执行一套全面的代码检查规则。我们还在推出 prettier 以使此过程更加自动化。
对于无法轻松自动化的事情，我们维护了一份样式指南，作者应遵守该指南，并且审查者在审查拉取请求时应牢记该指南。
除此之外，我们就进入了主观领域。诸如“这不是很可读”之类的陈述几乎没有帮助，因为它们无法被限定，但这并不意味着审查者应完全忽略由于其编写方式而难以理解的代码。没有一种明确“最佳”的方式来编写任何特定的代码，因此追求这种方式不应是我们的目标。相反，审查者和作者都必须接受，可能有许多不同的适当方式来使用代码完成相同的事情，只要贡献使用其中一种方式，那么我们就做得很好。
另一方面，如果审查者指出某段特定的代码难以理解，那么第一个查看该代码的人很难理解它。如果审查者强烈认为这是一个问题，他们应该提出该问题供作者考虑，并可能提出替代方案。审查者必须务实，他们的目标应该是推动讨论向前发展，而不是简单地设置障碍。
如有疑问，依靠代码库中的“现有技术”，尤其是在正在贡献的代码库周围的区域，通常是一个好主意。
有时，一个人可能会受到特定贡献的启发，从而引入一种新的代码样式，而我们已经有不同的样式指南或“现有技术”了。在拉取请求中提出这一点是可以的，但最终该讨论应分支到单独的 issue 或拉取请求中以更新相应的指南。如果此更改是由审查者提出的，则原始 PR 不应因此而被阻止。如果更改是由作者提出的，则他们可以选择更新 PR 以与我们现有的指南保持一致（首选），或者他们可以选择完全阻止 PR 进行单独的样式指南讨论。
吹毛求疵
编辑吹毛求疵是指审查者在拉取请求中识别出琐碎且不重要的细节，并要求作者对其进行更改。这是一个完全主观的类别，无法进行普遍定义，并且对吹毛求疵制定每个人都会满意的统一政策也同样不切实际。
审查者应放心给出他们对拉取请求的任何反馈，无论该反馈多么微不足道。作者也应该同样放心忽略他们认为琐碎且无关紧要的反馈。
通常，审查者对他们即将给出的反馈是否是吹毛求疵有自己的看法。虽然不是必需的，但将该反馈标识为吹毛求疵会很有帮助，例如“nit：此后添加一个换行符会很有帮助”。这有助于作者了解你的意图。
处理分歧
编辑审阅者和作者之间出现意见冲突是常有的事，有时很难调和这些意见。理想情况下，大家可以本着这些指导原则的精神共同努力达成共识，但如果行不通，最好引入第三方参与讨论。我们的拉取请求通常有两名审阅者，因此合适的第三方可能已经很明显了。否则，请联系最合适的功能领域，如果领域不明确，则联系技术领导。
不恰当的审阅反馈
编辑一条反馈是否适合用于拉取请求，通常取决于最初给出反馈的动机。
要求作者主要基于“我会如何编写这段代码？”的想法进行更改是不合适的。审阅者没有编写代码，他们在审阅过程中的关键目的不是将贡献内容修改成他们自己编写的代码的样子。如果审阅者想要提供这种类型的反馈，他们应该将其限定为“小问题”，如“挑刺”部分所述，以明确表示作者可以接受或忽略它。
诸如“这太烂了”之类的煽动性反馈根本不是反馈。它既刻薄又无益，而且永远是不恰当的。
检查清单
编辑为所有可能的拉取请求中应该发生的所有事情建立一个全面的检查清单是不切实际的，但这并不意味着我们缺乏可以列举的一套具体的最低要求。以下项目应该对任何拉取请求进行双重检查
- CLA检查通过
- CI作业运行并通过
- 遵守我们各种风格指南的精神
- 具有全面的单元测试覆盖率
- 自动化测试提供高度信心，确保更改在没有人工验证的情况下继续工作
- 包含适当的产品文档（asciidocs）
- 任何新的UI更改都对残疾人士是可访问的，包括但不限于颜色有足够的对比度、键盘导航和aria标签
- 在适当的情况下采取了足够的安全保护措施，尤其是在呈现API响应、解析表达式、编译URL、加载外部内容或重定向时
- PR标题总结了更改（不是“修复bug编号123”）
-
PR描述包括
- 对更改内容的详细总结
- 更改的动机
- 如果UI发生更改，则包含屏幕截图
- 指向PR关闭的每个问题的链接（例如，Closes #123）