mirror of
https://github.com/RIOT-OS/RIOT.git
synced 2024-12-29 04:50:03 +01:00
doc: add MAINTAINING.md
- This document is the result of original work by @haukepetersen and refinement by @miri64, @aabadie, @kaspar030, @jcarrano, @PeterKietzmann, @ZetaR60, and @jia200x.
This commit is contained in:
parent
42c4460daf
commit
849d20966e
131
MAINTAINING.md
Normal file
131
MAINTAINING.md
Normal file
@ -0,0 +1,131 @@
|
||||
# RIOT Maintainer Guidelines
|
||||
|
||||
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.
|
||||
|
||||
The [list of maintainers] gives information on the current maintainers and the
|
||||
areas of RIOT they each maintain.
|
||||
|
||||
|
||||
## Technical guidelines
|
||||
|
||||
### 1. - Review the fundamentals
|
||||
|
||||
Before spending the time on an in-depth code review, it's important to assess
|
||||
the overall validity of the PR.
|
||||
|
||||
1. 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?
|
||||
2. Is the solution presented in the PR as simple as possible to satisfy the
|
||||
requirements, but no simpler?
|
||||
3. Is the solution well designed on a high level?
|
||||
4. Do the concepts used by the PR make sense?
|
||||
5. Does the PR break with existing concepts?
|
||||
6. Is the structure of the PR itself valid?
|
||||
7. 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.
|
||||
8. Does the code compile and run?
|
||||
9. Does this PR respect the rights of previous authors, either through
|
||||
retaining their commits or by retaining their copyrights in the boilerplate
|
||||
headers?
|
||||
10. Is the PR a duplicate of another PR?
|
||||
|
||||
|
||||
### 2. - Review the design of the code
|
||||
|
||||
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.
|
||||
|
||||
1. Check for code duplication
|
||||
2. [Check memory usage][Comparing build sizes]
|
||||
3. Check all code paths
|
||||
4. Check for API compliance
|
||||
5. Check for consistent error handling
|
||||
6. Check scope of variables and functions
|
||||
7. Check for syntactical, semantical or logical errors
|
||||
8. 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
|
||||
9. Check that the code design is as simple as possible to solve the problem,
|
||||
but no simpler
|
||||
|
||||
|
||||
### 3. - Test the PR
|
||||
|
||||
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.
|
||||
|
||||
|
||||
### 4. - Review the code against the coding conventions
|
||||
|
||||
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
|
||||
subjective and is allowed subject to the other guidelines, and the coding
|
||||
conventions, which are absolute and must always be followed.
|
||||
|
||||
|
||||
### 5. - Review the documentation
|
||||
|
||||
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.
|
||||
|
||||
1. Check for sufficient high-level (module-level) documentation
|
||||
2. Verify function level documentation
|
||||
3. Are critical/hard to understand parts in the code documented?
|
||||
4. Check grammar and spelling of documentation
|
||||
|
||||
|
||||
## Non-technical guidelines
|
||||
|
||||
### Interaction with contributors
|
||||
|
||||
- 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
|
||||
awesome 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. Be objective in your review, and of course, follow the
|
||||
[Code of Conduct].
|
||||
|
||||
|
||||
### Organisation of reviewing between maintainers
|
||||
|
||||
This section is a placeholder for further discussion around making the review
|
||||
process more efficient and effective when multiple maintainers are involved.
|
||||
This could include:
|
||||
|
||||
- Usage of labels
|
||||
- Division of review responsibilities, e.g. with ACKs for different areas of
|
||||
review
|
||||
- Usage of GitHub functionality, for example "Reviewers" and "Assignees" lists
|
||||
|
||||
[list of maintainers]: https://github.com/RIOT-OS/RIOT/wiki/Maintainers
|
||||
[Best Practices]: https://github.com/RIOT-OS/RIOT/wiki/Best-Practice-for-RIOT-Programming
|
||||
[Comparing build sizes]: https://github.com/RIOT-OS/RIOT/wiki/Comparing-build-sizes
|
||||
[Coding Conventions]: https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions
|
||||
[Code of Conduct]: https://github.com/RIOT-OS/RIOT/blob/master/CODE_OF_CONDUCT.md
|
Loading…
Reference in New Issue
Block a user