Dear community:
As we continue to grow the codebase and community, it would be helpful for us to also update code review guideline to help us get common ground in the healthy collaborations.
This thread proposes to update the code review guideline to
@jroesch, myself and many others drafted a proposed update capture some of the best practices in open source and lessons we learnt in the past. The goal is of course to serve as living document for the lessons, we learnt and continue to update the process. Please share your thoughts.
Code Review Guideline
Open source code is maintained by a community with diverse backgrounds, interests, and goals. Hence it is important to provide clear, documented and maintainable code and processes. Code reviews are a shepherding process used to collectively spot potential problems, improve quality of the code, and educate both contributors and reviewers about the code base and its assumptions. It is also one mechanism to ensure there are multiple people who can maintain a related piece of code together. Contributors are encouraged to polish the code to a reviewable state before requesting reviews. This is especially important for committer candidates, as committers are expected to participate in not only writing code but also reviewing it.
This document is a living guideline for code review in open source.
Building Trust
First and foremost, we are building a community that based on trust, which takes time and effort to both build and maintain. We expect our community members to work together in a constructive way and work together with common sense. Although we all have different sets of backgrounds, interests and goals we must work together to find solutions that work for the larger community. Trust-based collaboration is also a key tenant of the Apache way and an important factor to consider in growing the community, and promoting members to official roles.
Community Participation
Everyone is welcomed to comment on PRs. We encourage committers to wait for some period of time(e.g. three days) before merging PR that contains a major architecture change. The goal is to give people time to speak up and express interest in reviewing and participate.
Remembering that we are all coming from different backgrounds is important here. For example some community members work in different time zones, only work on open source during work hours, or may be traveling or having other events going on in their lives. An important part of working in a large project is ensuring there is collective understanding, so no one person is a bottleneck. While it is important to allow time for participation in code review we also can not block all changes on all reviewers. Remember that helping people land PRs is a great way to encourage broader participation, especially for those who volunteer their time to contribute.
Part of this is trusting and communicating with fellow maintainers that if changes need to be applied in the future that PR authors will later follow through on their promises. It is the responsibility of committers to listen to all feedback whether from PMC members or new contributors and consider what actions need to be taken.
Read the code carefully
Sometimes we may quickly read through the code and only pick up on a selective aspects of the code. These type of comments are usually helpful and should be welcomed in the community. However, they are only part of performing code review and should be part of more comprehensive feedback. A good and careful code review is a large time investment and sometimes can be longer than writing the actual contribution.
For example receiving only highly critical feedback on minor aspects of your PR rarely feels good, and it can be discouraging if your time and effort was not reciprocated during review. Practicing empathy when acting both as a contributor and committer is important and can help make you a more effective code reviewer and contributor.
We expect that all committers carefully read and understand the code before signing off. There is a lot of trust involved when a committer hits the merge button. In the meantime, we acknowledge that sometimes problems slip through, in that case, the merger is responsible for ensuring the correct follow up actions are taken.
Be Respectful
-
To everyone who are making comments: making constructive comment will help new contributors to land their PRs timely and help us welcome new members to the community.
-
To authors: reviewers should spend significant time reading the code, and a careful review could be as time intensive as writing the code from scratch. Respectfully address review comments and reciprocate the review by helping review others changes in the future.
Most importantly focus on having a constructive conversation, and try to assume best intentions when interacting as a reviewer. If there is something in the process not working, consider getting some face time with the other contributors and discussing how to improve the process or communication.
Factors to Consider about Code Quality
High quality code is critical to the long term success of the project. There are many factors of code quality to consider during a code review:
-
F0: Overall architecture. This includes the definition of public modules, key data structures and public interfaces. Good architectural choices are critical to the success of the project in the long run.
-
F1: Architectural consistency. There are usually multiple ways to implement a new feature. We must ensure new features are consistent with previous overall architectural choices and interact well with the existing code. Every new feature increases the complexity of the project, and a consistent design ideally minimizes the increase in complexity bought by a new feature, making it easier to maintain code in the long run.
-
F2: Code robustness and test coverage. Ensure code runs correctly in all possible settings(platforms), ensure test coverage of the new feature. Clear error messages for user facing errors.
-
F3: User facing API documentation: documentation of public user facing APIs and key module interfaces are mandatory. This includes the API, data structures that appears in the public interface (i.e.,
include/tvmand user facing python APIs). We generally encourage well documented code and include some form of documentations for internal APIs that are used in multiple places, see also F4. -
F4: Code readability. Readability involves multiple aspects: instructive and consistent function names, clear implementation of the overall flow, descriptive comments for complex code logic and internal functions. Readable code is easier to maintain.
Architectural design and consistency are the most important factors since they are likely to introduce the most long term technical debt. As a result, committers should most carefully consider these factors before merging the code.
Test coverage and API documentation are expected for code contributions.
Code readability is relatively a subjective matter compared to the other ones. Different people have different thoughts on how to best write code. Reviewers should make constructive and actionable comments. In the meantime, code review should not be used as a way to get others to write code exactly the way you would. Conversely you should also consider that what you may easily understand, or find acceptable might not work for the larger community or other members. Use your judgment on what is appropriate based on the content and the scope of the contribution and where the contributor is coming from.
We follow common code style guides when writing code. Style guides help ensure that code is readable and maintainable by others, long after the original author has moved on. Style guides are more than about code formatting ā they also pertain to the correct way to document code, variable naming, and other conventions that are not enforced by automatic formatters.
Consensus Building
Disagreements can happen during code reviews. We encourage building consensus among the people involved. We are working together and building trust with each other in OSS. The nature of OSS means sometimes we make compromises on less significant issues to make steady progress and welcome broader participation in the community. Compromise unfortunately means sometimes the world will not be exactly as we would like, this true even for leaders of the community.
-
Be civil and build consensus through constructive technical-based conversations.
-
A committer who owns the area can serve as a shepherd to drive the discussion by taking all the conversations into consideration, and suggest a resolution with to move forward.
-
Because a lot of trust is involved on the committer(shepherd), they should read the PR carefully before sign off. Additionally, the merger should also take the responsibility to followup in case there are problems caused by the merge.
Consistency
A final remark is that we are all human and its hard to always be perfectly consistent. If contributors feel that you didnāt apply these guidelines in a consistent way it is important to listen and hear folks out. We will constantly have to iterate on processes and guidelines as we evolve as a community. Our goal is to strive to be consistent and objective but all of us are unfortunately human and imperfect and will need to adjust and learn.
Additional Recommendations
Scope the PRs
We recommend authors to send well scoped PRs that are easy to review and revert in case there is a problem. Authors avoid merging multiple unrelated changes into a single PR and split them into separate PRs.
Label the PRs with Area Prefix
When sending pull requests, it is helpful to prefix the PR title with the areas related PR(e.g. use [TIR] for TIR-related changes). This would help people recognize the related areas and find PRs they are interested in.
Append Other existing tips in code review docs
.