mirror of
https://github.com/LCTT/TranslateProject.git
synced 2024-12-26 21:30:55 +08:00
Merge pull request #28493 from yzuowei/yzuowei-translating-20221031.1
translated
This commit is contained in:
Xingyu.Wang
GitHub
commit
0d02a68ce7
@ -1,134 +0,0 @@
|
|||||||
[#]: subject: "10 universal steps for open source code review"
|
|
||||||
[#]: via: "https://opensource.com/article/22/10/code-review"
|
|
||||||
[#]: author: "Martin Kopec https://opensource.com/users/martin-kopec"
|
|
||||||
[#]: collector: "lkxed"
|
|
||||||
[#]: translator: "yzuowei"
|
|
||||||
[#]: reviewer: " "
|
|
||||||
[#]: publisher: " "
|
|
||||||
[#]: url: " "
|
|
||||||
|
|
||||||
10 universal steps for open source code review
|
|
||||||
======
|
|
||||||
|
|
||||||
Code review doesn't have to be scary when you follow this universal process.
|
|
||||||
|
|
||||||
Have you ever found yourself in a situation where you needed to do a code review but didn't fully understand the project? Maybe you did not review it to avoid looking like you didn't know what you were doing.
|
|
||||||
|
|
||||||
This article assures you that there's a better way. You don't need to know everything to provide a code review. In fact, based on my experience, that's quite common.
|
|
||||||
|
|
||||||
I remember when I joined Red Hat as an intern and was asked to help with code reviews. We used a system of +1 or -1 votes, and I was initially very hesitant to weigh in. I found myself asking whether when I gave a +1 on a change but then someone else voted -1, would I look foolish?
|
|
||||||
|
|
||||||
What does happen if someone votes -1 on a change you've vote +1? The answer is nothing! You might have missed a detail that the other person noticed. It's not the end of the world. That's why we have this voting system. Like the rest of open source, merging code is a collaborative effort.
|
|
||||||
|
|
||||||
Lately, I've been so inundated with code reviews that I can hardly keep up with them. I also noticed that the number of contributors doing these reviews steadily decreased.
|
|
||||||
|
|
||||||
For this reason, I'm writing about my point of view on writing a code review. In this article, I'll share some helpful tips and tricks. I'll show you a few questions you should ask yourself and a few ideas of what to look for when doing a code review.
|
|
||||||
|
|
||||||
### What is the purpose of a code review?
|
|
||||||
|
|
||||||
Have you ever written a really simple patch? Something you think is so trivial that it doesn't require a review? Maybe you merged it straight away. Later, it turns out there was a mistake, something obvious or silly, like the wrong indentation or a few duplicated lines of code instead of a function call (yes, I'm speaking from experience!).
|
|
||||||
|
|
||||||
A code review by someone else would have caught these things.
|
|
||||||
|
|
||||||
The point of a code review is to bring a fresh pair of eyes with a new perspective on the problem you're trying to solve. That new context is exactly the reason a code review is crucial.
|
|
||||||
|
|
||||||
You may think that you must be an expert in the language to review someone else's code, the project, or both. Here's a secret all code reviewers want you to know: That's wrong! You don't need to fully understand the project or the language to provide a fresh perspective on a change. There's a universal process of code review.
|
|
||||||
|
|
||||||
### The universal process of a code review
|
|
||||||
|
|
||||||
Here's my process for code review, grouped into a couple of points. The process provides questions I ask myself to help me focus on a code change and its consequences. You don't need to go in this specific order. If there's a step, you can't execute for any reason, just move to another step.
|
|
||||||
|
|
||||||
### 1. Understand the change, what it's trying to solve, and why
|
|
||||||
|
|
||||||
The explanation of why the change is needed and any relevant context should be in the commit message. If it isn't, request it and feel free to -1 until it's provided.
|
|
||||||
|
|
||||||
Is it something that needs to be solved? Is it something the project should focus on, or is it completely out of scope?
|
|
||||||
|
|
||||||
### 2. How would you implement the solution? Would it be different?
|
|
||||||
|
|
||||||
At this point, you know what the code change is about. How would you have done it? Think about this before reviewing the change in detail. If the solution you have in mind is different from the one you're reviewing, and you think it's better, bring that up in the review. You don't need to -1 it; just ask why the author didn't go in this direction and see how the discussion evolves.
|
|
||||||
|
|
||||||
### 3. Run the code with and without the change
|
|
||||||
|
|
||||||
I usually put a few breakpoints into the code, run it, and inspect how the new code interacts with the rest.
|
|
||||||
|
|
||||||
If you can't run the whole code, try to copy the function containing the new code to a new local file, simulate the input data, and run that. This is helpful when you either don't know how to run the whole project or when it requires a specific environment to which you don't have access.
|
|
||||||
|
|
||||||
### 4. Can the new code break anything?
|
|
||||||
|
|
||||||
I mean, really anything. Think about the consequences.
|
|
||||||
|
|
||||||
In the case of a new command-line option, will it always be accepted by the target?
|
|
||||||
|
|
||||||
Can a situation occur when the option wouldn't be accepted or when it could conflict with something?
|
|
||||||
|
|
||||||
Maybe it's a new import. Is the new library, and possibly a new dependency, available in the older releases or systems you ship the project for?
|
|
||||||
|
|
||||||
What about security? Is the new dependency safe to use? The least you can do is run a quick Internet search to find out. Also, look for warnings in the console log. Sometimes there are more secure methods within the same library.
|
|
||||||
|
|
||||||
### 5. Is the code effective?
|
|
||||||
|
|
||||||
You've determined that the proposed solution is probably correct. Now it's time to check the code itself, its effectiveness, and its necessity.
|
|
||||||
|
|
||||||
Check the style of the new code. Does it match the style of the project? Any open source project has (or should have) a document informing (new) contributors about the styles and good practices the project follows.
|
|
||||||
|
|
||||||
For instance, every project in the OpenStack community has a HACKING.rst file. There's often also [a guide for new contributors][1] with all the must-know information.
|
|
||||||
|
|
||||||
### 6. Check that all new variables and imports are used
|
|
||||||
|
|
||||||
Often, there have been many iterations of the code you're reviewing, and sometimes the final version is very different from when it started. It's easy to forget an import or a new variable that was needed in a former version of the new code. Automation usually checks these things using linting tools like [flake8][2] in the case of Python code.
|
|
||||||
|
|
||||||
Can you rewrite the code without declaring new variables? Well, usually, yes, but the question is whether it's better that way. Does it bring any benefit? The goal isn't to create as many one-liners as possible. The goal is to write code that is both efficient and easy to read.
|
|
||||||
|
|
||||||
### 7. Are the new functions or methods necessary?
|
|
||||||
|
|
||||||
Is there a similar function that can be reused somewhere in the project? It's always worth helping to avoid reinventing the wheel and re-implementing logic that's already been defined.
|
|
||||||
|
|
||||||
### 8. Are there unit tests?
|
|
||||||
|
|
||||||
If the patch adds a new function or new logic in a function, it should also include new unit tests for that. It's always better when the author of a new function also writes unit tests for it.
|
|
||||||
|
|
||||||
### 9. Verify refactoring
|
|
||||||
|
|
||||||
If the commit refactors existing code (it renames a variable, changes variable scope, changes the footprint of a function by adding or removing arguments, or removes something), ask yourself:
|
|
||||||
|
|
||||||
- Can this be removed? Will it affect the stable branch?
|
|
||||||
- Are all the occurrences deleted?
|
|
||||||
|
|
||||||
You can use the [grep command][3] to find out. You wouldn't believe how many times I've voted -1 just because of this. This is a simple mistake that anyone can make, but that also means anyone can uncover it.
|
|
||||||
|
|
||||||
The owner of the commit can easily overlook these things, which is totally understandable. It's happened to me many times too. I'd finally figured out the root of the problem I'd been fixing, so I was in a rush to propose the review, and then I forgot to check the whole repo.
|
|
||||||
|
|
||||||
Apart from the project's repository, sometimes it's also necessary to check other code consumers. If some other project imports this one, they may need refactoring, too. In the OpenStack community, we have a tool that searches across every community project.
|
|
||||||
|
|
||||||
### 10. Does project documentation need to be modified?
|
|
||||||
|
|
||||||
Again, you can use the [grep command][4] to check whether the project documentation mentions anything related to the code change. Apply common sense to determine whether a change needs to be documented for end users or it's just an internal change that doesn't affect user experience.
|
|
||||||
|
|
||||||
### Bonus tip: Be considerate
|
|
||||||
|
|
||||||
Be considerate, precise, and descriptive if you make a suggestion or comment on something after you've reviewed the new code. Ask questions if you don't understand something. If you think the code is wrong, explain why you think so. Remember, the author can't fix it if they don't know what's broken.
|
|
||||||
|
|
||||||
### Final words
|
|
||||||
|
|
||||||
The only bad review is no review. By reviewing and voting, you provide your point of view and vote only for that. Nobody expects you to give the final yes or no (unless you're a core maintainer!), but the voting system allows you to provide your perspective and opinion. A patch owner will be glad you did it, trust me.
|
|
||||||
|
|
||||||
Can you think of any other steps for a good review? Do you have any special technique different from mine? Let us all know in the comments!
|
|
||||||
|
|
||||||
--------------------------------------------------------------------------------
|
|
||||||
|
|
||||||
via: https://opensource.com/article/22/10/code-review
|
|
||||||
|
|
||||||
作者：[Martin Kopec][a]
|
|
||||||
选题：[lkxed][b]
|
|
||||||
译者：[译者ID](https://github.com/译者ID)
|
|
||||||
校对：[校对者ID](https://github.com/校对者ID)
|
|
||||||
|
|
||||||
本文由 [LCTT](https://github.com/LCTT/TranslateProject) 原创编译，[Linux中国](https://linux.cn/) 荣誉推出
|
|
||||||
|
|
||||||
[a]: https://opensource.com/users/martin-kopec
|
|
||||||
[b]: https://github.com/lkxed
|
|
||||||
[1]: https://docs.openstack.org/tempest/latest/contributor/contributing.html
|
|
||||||
[2]: https://opensource.com/article/19/5/python-flake8
|
|
||||||
[3]: https://opensource.com/downloads/grep-cheat-sheet
|
|
||||||
[4]: https://www.redhat.com/sysadmin/how-to-use-grep
|
|
||||||
@ -0,0 +1,137 @@
|
|||||||
|
[#]: subject: "10 universal steps for open source code review"
|
||||||
|
[#]: via: "https://opensource.com/article/22/10/code-review"
|
||||||
|
[#]: author: "Martin Kopec https://opensource.com/users/martin-kopec"
|
||||||
|
[#]: collector: "lkxed"
|
||||||
|
[#]: translator: "yzuowei"
|
||||||
|
[#]: reviewer: " "
|
||||||
|
[#]: publisher: " "
|
||||||
|
[#]: url: " "
|
||||||
|
|
||||||
|
开源代码评审的十个通用步骤
|
||||||
|
======
|
||||||
|
|
||||||
|
只要你遵循这些通用流程，<ruby>代码评审<rt>code review</rt></ruby>并不可怕。
|
||||||
|
|
||||||
|
你是否曾需要对代码进行评审，但你还没有完全理解整个项目？或许你避开去评审从而避免你看起来像是什么都不知道的洋相。
|
||||||
|
|
||||||
|
本篇文章想要告诉你一个更好的方法。代码评审并不需要你知道所有事情。实际上，就我个人经验而言，这种情况非常普遍。
|
||||||
|
|
||||||
|
我还记得在我刚加入<ruby>红帽<rt>Red Hat</rt></ruby>做实习的时候，我被要求参与代码评审。我们当时采取的是 +1 或 -1 的投票系统，而我在一开始的时候常常踌躇该如何评审。我发现我总是问自己对于一处改动是否应该给予 +1 当别人已经投了 -1，我会看起来很蠢吗？
|
||||||
|
|
||||||
|
如果你对一处改动投了 +1， 但是别人又投了 -1，这又意味着什么呢？这不意味任何事！你可能只是漏掉了一处细节而别人又恰好注意到了。这不意味着世界末日。这也是为什么我们会用投票系统。正如同所有开源项目一样，代码合并是一项协同工作。
|
||||||
|
|
||||||
|
最近，我接到了太多的代码评审工作以至于我难以维持进度。我同时也注意到了参与评审的贡献者数量正在稳步减少。
|
||||||
|
|
||||||
|
基于这个原因，我想要写一篇文章阐述我对代码评审的个人观点。在这篇文章里，我会分享一些诀窍与技巧。我会向你展示几个问题你可以用来自问自答，我也会为在评审代码时需要注意什么提供一些想法。
|
||||||
|
|
||||||
|
### 代码评审的目的是什么？
|
||||||
|
|
||||||
|
你是否曾写过一个非常简单的补丁？补丁太过琐碎以至于你认为评审是多余的？或许你马上进行了合并。直到晚些时候，你意识到你犯了个错误，一个明显或是愚蠢的错误，或是错误缩进，或是几行重复的代码在本可以调用函数的地方（是的，这些都是经验之谈！）。
|
||||||
|
|
||||||
|
一段代码本可以交予他人评审并及时发现这些问题。
|
||||||
|
|
||||||
|
代码评审的一个目的便是为你在尝试解决的问题带来一双全新的视角。新开阔的视野也正是为什么代码评审至关重要。
|
||||||
|
|
||||||
|
你可能会认为评审别人的代码需要你是一名专家，或是编程语言专家，或是项目专家，或两者兼具。让我来告诉你一个所有代码评审者都想跟你说的秘密吧：大错特错！你并不需要完全理解项目或者编程语言来为改动提供全新视角。我将向你展示代码评审的通用流程。
|
||||||
|
|
||||||
|
### 代码评审的通用流程
|
||||||
|
|
||||||
|
这是我的代码评审流程被拆分成几个要点。这个流程包含了我会问我自己的一些问题，以帮助我专注于代码的变化以及其后果。你不需要严格依照顺序来进行评审。如果有任何原因导致你无法执行其中的某一步，跳过那一步就好。
|
||||||
|
|
||||||
|
### 1. 理解改动，问改动想要解决的问题，以及为什么要解决这个问题
|
||||||
|
|
||||||
|
为什么需要改动的解释以及任何相关背景都应该被放在<ruby>提交<rt>commit</rt></ruby>信息里。如果没有，要求相关信息并请随意投 -1 直到相关信息被提供。
|
||||||
|
|
||||||
|
改动想解决的问题需要被解决吗？它是项目应当聚焦的问题，还是与项目完全无关？
|
||||||
|
|
||||||
|
### 2. 你会如何实现解决方案？它会不一样吗？
|
||||||
|
|
||||||
|
在这个时候，你应该已经知道代码改动是为了什么。换做是你会怎么做？在进一步对改动进行细节评审前思考这个问题。如果你想出了一个不一样的解法，并且你认为你的解法更好，在评审中提出来。你不需要投 -1；去问问作者为什么没有往那个方向走，看看这次讨论会把你们带向何方。
|
||||||
|
|
||||||
|
### 3. 运行有改变和无改变的代码
|
||||||
|
|
||||||
|
我通常会在代码中设置几个断点，运行代码并检查新代码是如何与其余部分互动的。
|
||||||
|
|
||||||
|
如果你无法运行整个代码，试着将带有新代码的函数复制到一个新的本地文件，模拟输入数据，然后运行。这在你不知道怎么运行整个项目或者无法接触到运行所需的特殊环境时很有帮助。
|
||||||
|
|
||||||
|
### 4. 新代码会破坏任何东西吗？
|
||||||
|
|
||||||
|
我是说，任何东西。想一想可能的后果。
|
||||||
|
|
||||||
|
以一个新的命令行选项为例，它会总是被目标所接受吗？
|
||||||
|
|
||||||
|
是否存在这样一种情况使得新选项无法被接受或是会与其他东西起冲突？
|
||||||
|
|
||||||
|
或许新代码是一个新的导入。那么这个新的库，很可能也是新的依赖，能够在老版本或者项目的运行系统中被找到吗？
|
||||||
|
|
||||||
|
安全方面呢？新的依赖足够安全吗？你至少可以在网上快速地搜索一下。还有，注意一下控制台日志里的警告。有的时候在同样的库里也可以找到更安全的方法。
|
||||||
|
|
||||||
|
### 5. 新代码是否有效？
|
||||||
|
|
||||||
|
你刚刚确认了被提出的解决方案大概是正确的。现在该检查代码本身了。你需要关注代码的有效性和必要性。
|
||||||
|
|
||||||
|
检查新代码的风格。它与项目的代码风格相匹配吗？任何开源项目都（应该）有一份文档告知（新）贡献者项目所遵循的风格和优秀实践。
|
||||||
|
|
||||||
|
比如说，OpenStack 社区的所有项目都有一份 HACKING.rst 文件。你经常也能找到一份[新贡献者指南][1]包含所有必须知道的信息。
|
||||||
|
|
||||||
|
### 6. 确认所有新增的变量和导入都被使用
|
||||||
|
|
||||||
|
你正在评审的代码常常已经过多次迭代，有的时候代码的最终版本与初始版已迥然不同。所以我们很容易忘记一些在历史版本中加入的变量与引用。自动化检测通常会用到 lint 工具，类似 Python 中的 [flake8][12]。
|
||||||
|
|
||||||
|
（LCTT 译注：[lint][5] 指编程中用来发现代码潜在错误和约束代码风格的工具，起源于 C 语言编程中的静态分析工具 lint。lint 本意为衣服上积累的绒毛与灰尘，lint 的取名寓意则在于捕捉编程时产生的“绒毛与灰尘”）
|
||||||
|
|
||||||
|
你可以在不声明新变量的情况下重写代码吗？通常情况下你可以，但问题是这样是否更好。这会带来什么益处吗？我们的目标不是多写花式的一行代码，而是写出高效且易读的代码。
|
||||||
|
|
||||||
|
### 7. 新的函数和方法是否必要？
|
||||||
|
|
||||||
|
项目里的别的地方是否存在可以被复用的功能类似的函数？确保避免重新发明轮子以及重新实现已经被定义的逻辑永远都是值得的。
|
||||||
|
|
||||||
|
### 8. 有单元测试吗？
|
||||||
|
|
||||||
|
如果补丁增加了新的函数或者在函数内添加了新的逻辑，它也应该附带对应的单元测试。新函数的作者总是比别人更适合写该函数的单元测试。
|
||||||
|
|
||||||
|
### 9. 验证重构
|
||||||
|
|
||||||
|
如果这次提交对现有代码进行了重构（它可能重命名了某个变量，或者是改变了的变量的作用域，或者是通过加减参数来改变函数的足迹，又或者是删去了某个东西），问一问你自己：
|
||||||
|
|
||||||
|
- 这个可以被删除吗？它会影响到稳定分支吗？
|
||||||
|
- 所有出现的地方都删掉了吗？
|
||||||
|
|
||||||
|
你可以利用 [grep 命令][3]来查找。你不会相信有多少次我投 -1 就是因为这个。这是一个任何人都会犯的错误，也正因如此任何人都可以发现它。
|
||||||
|
|
||||||
|
提交的所有者很容易忽略这些事情，这完全可以理解。我也犯过很多次这种错误。我最终发现问题的根源在于我太急于提出评审，以至于我忘记了对仓库进行整体检查。
|
||||||
|
|
||||||
|
除了对项目仓库的检查外，检查其他代码用户也十分必要。如果有别的项目导入了这个项目，它们可能也需要进行重构。在 OpenStack 社区中，我们有对应的工具来查询别的社区项目。
|
||||||
|
|
||||||
|
### 10. 项目文档是否需要做出更改？
|
||||||
|
|
||||||
|
你可以再一次使用 [grep 命令][4]来检查在项目文档中是否提到了相关的代码改动。用常识来判断这次改动是否需要被收入文档以告知最终用户，还是只是一个不会影响用户体验的内部变化。
|
||||||
|
|
||||||
|
### 额外提示：考虑周到
|
||||||
|
|
||||||
|
当你在评审完新代码后提出建议或评论时，要考虑周到，反馈准确，描述详尽。如果有你不理解的地方就发出提问。如果你认为代码存在错误，解释你的理由。记住，作者无法修复他不知道的漏洞。
|
||||||
|
|
||||||
|
### 最后几句
|
||||||
|
|
||||||
|
唯一的坏评审是没有评审。通过评审和投票，你提供了你的观点并为此发声。没有人指望你来做出最终决定（除非你是核心维护者），但是投票系统允许你提供你的观点和意见。相信我，补丁所有者会很高兴你这么做了的。
|
||||||
|
|
||||||
|
你能想到别的要点来给出好的评审吗？你是否有我不知道的特殊技巧？在评论中分享它们吧！
|
||||||
|
|
||||||
|
--------------------------------------------------------------------------------
|
||||||
|
|
||||||
|
via: https://opensource.com/article/22/10/code-review
|
||||||
|
|
||||||
|
作者：[Martin Kopec][a]
|
||||||
|
选题：[lkxed][b]
|
||||||
|
译者：[yzuowei](https://github.com/yzuowei)
|
||||||
|
校对：[校对者ID](https://github.com/校对者ID)
|
||||||
|
|
||||||
|
本文由 [LCTT](https://github.com/LCTT/TranslateProject) 原创编译，[Linux中国](https://linux.cn/) 荣誉推出
|
||||||
|
|
||||||
|
[a]: https://opensource.com/users/martin-kopec
|
||||||
|
[b]: https://github.com/lkxed
|
||||||
|
[1]: https://docs.openstack.org/tempest/latest/contributor/contributing.html
|
||||||
|
[2]: https://opensource.com/article/19/5/python-flake8
|
||||||
|
[3]: https://opensource.com/downloads/grep-cheat-sheet
|
||||||
|
[4]: https://www.redhat.com/sysadmin/how-to-use-grep
|
||||||
|
[5]: https://codedocs.org/what-is/lint-software
|
||||||
Loading…
Reference in New Issue
Block a user