This list presents a series of guidelines for maintainers, including technical and non-technical. The list is not exhaustive, and represents a baseline on things that should be ensured for contributions.
Notes:
- Any of the items in this list might be skipped, if clearly and logically articulated reasons are presented.
- The order of the steps is meant to maximize efficiency and minimize overhead, with the philosophy that failing in step n makes steps n+x obsolete. However, this efficiency can depend on the quality of the submission itself. If the PR (Pull Request) is clearly not in a reviewable state (for example, due to poor code cleanliness or overall design), it might be more efficient to give the contributor some broader comments for improvement before further review.
- This is a working document and any changes, additions, or other discussions are encouraged, via PRs raised against the document. Changes to this document should however have at least two ACKs, to ensure these guidelines are well thought out, and that they reflect community consensus.
The list of maintainers gives information on the current maintainers and the areas of RIOT they each maintain.
Before spending the time on an in-depth code review, it's important to assess the overall validity of the PR.
- Does the reasoning for this PR make sense?
Are the requirements and design goals clearly thought out and clearly expressed?
Is the problem that the PR intends to solve clearly stated? - Is the solution presented in the PR as simple as possible to satisfy the requirements, but no simpler?
- Is the PR a manageable size? It should be confined to a single explainable change, and be runnable on its own.
- Is the solution well designed on a high level?
- Do the concepts used by the PR make sense?
- Does the PR break with existing concepts?
- Is there a clean commit history in the pulled branch? The commit history should group the code differences cleanly.
- Are there clear and adequate instructions on how to test the PR?
This may or may not include implemented tests as part of the PR. - Does the code compile and run?
- Does this PR respect the rights of previous authors, either through retaining their commits or by retaining their copyrights in the boilerplate headers?
- Is the PR a duplicate of another PR?
The following list is not exhaustive and addresses the coding issues we have regularly seen in the past. In particular, check that the Best Practices are followed. These checks can be aided (but not replaced) by a tool such as Coccinelle, using the script found in dist/tools/coccinelle.
- Check for code duplication
- Check memory usage
- Check all code paths
- Check for API compliance
- Check for consistent error handling
- Check scope of variables and functions
- Check for syntactical, semantical or logical errors
- Check for any runtime efficiency improvements that can be made in specific lines of code - i.e., without changing the overall design of the code
- Check that the code design is as simple as possible to solve the problem, but no simpler
Run tests to verify the correct behavior (see 1.6), both on native and on a
few selected boards, or present clearly and logically articulated reasons for
skipping some/all tests.
Check that the code follows the Coding Conventions. This can be aided (but not replaced) by Uncrustify, using the uncrustify-riot.cfg file found in the base directory. Note the difference between personal coding style, which is allowed subject to the other guidelines, and the coding conventions, which are absolute and must always be followed.
The aim of the documentation is to ensure that the code can be picked up as easily as possible in the future. Ideally, the documentation is sufficiently complete that no input from the original developer or maintainer is required.
- Check for sufficient high-level (module-level) documentation
- Verify function level documentation
- Are critical/hard to understand parts in the code documented?
- Check grammar and spelling of documentation
- Be responsive. Even if you are too busy to review the contribution, try to add a note fairly soon after the PR is submitted, thanking them for their valuable contribution and saying that you will review it in due course. Once the contributor has made their changes, ensure you reply to them in a reasonable timeframe. Acknowledge their replies to concerns if you're happy with their argument.
- Be helpful. Give precise and correct advice when possible and when it will help the contributor. This can include code snippets, links to code/issues/wiki entries/web pages, or anything else. Educating contributors means we are investing in our community.
- Be friendly. Respect the original author, bearing in mind that their coding style or their design may be just as valid as the way you would have done it. And of course, always follow the Code of Conduct.
You can review a PR partially. This would involve reviewing all points in one or more sections outlined in the technical guidelines. In that case, please do not "approve" the PR to prevent accidental merges. Rather, give your verbal ACK and describe what you reviewed. In addition, if you processed or reasonably stepped over a whole section, mark the PR with the according label from the "Reviewed:" category. If you set a label by stepping over a section, please articulate your reasoning for this clearly, as noted in the introduction. This will help other maintainers help to better understand your line of thought. If you disagree with the assessment of a previous review, you may remove a certain "Reviewed:" label. Please state your reasoning in this case as well.
When all "Reviewed:" labels are set you may give your approval for the PR.
As everything in this document this is a "CAN", not a "MUST": It might help other maintainers to track your work, but if the overhead isn't justified, a simple approving ACK might suffice.
It is good etiquette to describe what you reviewed, even if you gave the PR a full review and gave your approval. This way the contributor and other maintainers are able to follow your thought process.
Maintainers should only assign themselves to PRs and shouldn't assign other maintainers. You can however request reviews from other maintainers or contributors, either by mentioning them in a comment or selecting them in GitHub's review sidebar.
If there are multiple maintainers reviewing a PR, always give the other maintainers reasonable time to ACK before dismissing their review.