Pull requests guidelines
Wondering how to write a good PR up to the organisation's standards?
Pull requests are official record of all changes we make. As an open source organisation PRs might be read from multiple people and not only the assigned reviewers. Future contributors are likely to search and come back to past PRs for a variety of reasons, therefore, it is vital that the important changes are not only in the code but also included in the description. Always think about the reviewers and potential future readers!
Think twice about the title and description before submitting for review
The title is what usually appears in the version control. Thus, it must be a full, clear and imperative sentence.
Bad title example:
Deleting API endpoint and adding new one- not specific enough, not imperative, bad wording.
Good title example:
Delete the /foo/bar REST endpoint and replace it with /foo/baz
The latter example describes the specific change made, improves the searchability and gives enough information to the reader which allows them to skim through the changes rather than spending more time going through the rest of the description.
Keep the body as informative as possible. Start with outlining the problem you are trying to solve/the feature you want to add.
All readers must get a perfect impression of what the change is and reason/motivation behind it. Think about any attachments that might be useful. Add screenshots if the changeset leads to visual difference in the frontend, add any external link/references to any design docs that might help the reader to better understand context of/changes in the PR. However, try to keep those to a minimal and outline as much as possible in the written description as any external resources can easily be deleted in the future or problems with access permissions might occur.
Do not skip any important information. Outline with clarity the alternative solutions and the reasons for avoiding those. Similarly, it is vital to explain any shortcomings of the approach that you took to solving the problem, what can possibly go wrong in the future and how we can possibly overcome those issues if you can think of any.
Keep the changeset as small as possible
10 lines of code = 10 issues, 500 lines of code = LGTM!... code reviews in a nutshell...
Your PR changeset size should be just right, i.e. one self-contained change per PR! Furthermore, you should strive to limit your changeset to 250-300 lines of code where possible (source, source). This is beneficial for both the reviewers and authors. As a reviewer you will be able to go through the changes much quicker and provide a better feedback as the quantity of changes you are reviewing is less and the amount of context needed is proportional to the size. Vice-versa, as an author you will get good feedback fast and hoping for less potentially introduced bugs. Moreover, changes you will be required to make upon receiving the feedback would be less and easier to do, i.e. you will have to do less work if something is rejected. Small PRs are also easier to design and the mergeability and simplicity of rolling back the changes increase substantially.
How to introduce smaller PRs?
Break down your issues!
For example: you want to add a new feature for creating a product, your changes will include a new public API endpoint and its backend implementation. Instead of dumping all of your changes in a massive PR, you can break down your issue similarly to:
A differential with any Database schema changes required.
A differential with any protobuf changes required.
Initial commit with stubs for the backend.
A series of differentials containing the backend implementation and respective unit tests.
A differential with any changes required to the public API.
Stacking PRs greatly improves the efficiency of the above process, you can see more examples how to this here.
Tests for new functionality must go in the same PR as the functional changes, do not try to reduce the changeset size by splitting unit tests from functional changes.
Do not mix changes - refactoring, bug fixing and enhancements must not go in the same differential.
Large PRs
It is inevitable that sometimes larger PRs will be needed - when refactoring code, code added by automation, deleting files etc. This is always fine as long as you have considered all options how to avoid the massive change but nothing seems to help.
Do not break the builds!
The system's functionality must always be intact after merging the changes. We understand it is impossible for this rule not to be broken ever. But, introducing PRs that intentionally add a breaking change (even for a short period of time until your subsequent PR is merged) is unacceptable! There are a number of ways to avoid those and if unsure, ask your friendly community members for how to approach the problem.
Sources
The (written) unwritten guide to pull requests
Best Practices for Code Review
Last updated