Commit f16eae15 authored by Pekka Paalanen's avatar Pekka Paalanen

contributing: add review guidelines

This sets up the standards for patch review, and defines when a patch
can be merged. I believe these are the practises we have been using
already for a long time, now they are just written down explicitly.

It's not an exhaustive list of criteria and likely cannot ever be, but
it should give a good idea of what level of review we want to have.

It has been written in general terms, so that we can easily apply the
same text not just to Wayland, but also Weston and other projects as
necessary.

This addition is not redundant with
https://wayland.freedesktop.org/reviewing.html .

The web page is a friendly introduction and encouragement for people to
get involved. The guidelines here are more specific and aimed for people
who seek commit rights or maintainership.
Signed-off-by: Pekka Paalanen's avatarPekka Paalanen <pekka.paalanen@collabora.co.uk>
Reviewed-by: Matheus Santana's avatarMatheus Santana <embs@cin.ufpe.br>
Reviewed-by: Daniel Stone's avatarDaniel Stone <daniels@collabora.com>
Reviewed-by: default avatarEmil Velikov <emil.velikov@collabora.com>
Reviewed-by: Derek Foreman's avatarDerek Foreman <derek.foreman.samsung@gmail.com>
parent 35d0425e
...@@ -197,5 +197,54 @@ New source code files should specify the MIT Expat license in their ...@@ -197,5 +197,54 @@ New source code files should specify the MIT Expat license in their
boilerplate, as part of the copyright statement. boilerplate, as part of the copyright statement.
Review
======
All patches, even trivial ones, require at least one positive review
(Reviewed-by). Additionally, if no Reviewed-by's have been given by
people with commit access, there needs to be at least one Acked-by from
someone with commit access. A person with commit access is expected to be
able to evaluate the patch with respect to the project scope and architecture.
During review, the following matters should be checked:
- The commit message explains why the change is being made.
- The code fits the project's scope.
- The code license is the same MIT licence the project generally uses.
- Stable ABI or API is not broken.
- Stable ABI or API additions must be justified by actual use cases, not only
by speculation. They must also be documented, and it is strongly recommended to
include tests excercising the additions in the test suite.
- The code fits the existing software architecture, e.g. no layering
violations.
- The code is correct and does not ignore corner-cases.
- In a patch series, every intermediate step produces correct code as well.
- The code does what it says in the commit message and changes nothing else.
- The test suite passes.
- The code does not depend on API or ABI which has no working free open source
implementation.
- The code is not dead or untestable. E.g. if there are no free open source
software users for it then it is effectively dead code.
- The code is written to be easy to understand, or if code cannot be clear
enough on its own there are code comments to explain it.
- The code is minimal, i.e. prefer refactor and re-use when possible unless
clarity suffers.
- The code adheres to the style guidelines.
[git documentation]: http://git-scm.com/documentation [git documentation]: http://git-scm.com/documentation
[notes on commit messages]: http://who-t.blogspot.de/2009/12/on-commit-messages.html [notes on commit messages]: http://who-t.blogspot.de/2009/12/on-commit-messages.html
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment