aboutsummaryrefslogtreecommitdiffstats
path: root/docs/development/reviewing-patches.rst
diff options
context:
space:
mode:
authorAlex Gaynor <alex.gaynor@gmail.com>2014-02-11 21:32:11 -0800
committerAlex Gaynor <alex.gaynor@gmail.com>2014-02-11 21:32:11 -0800
commit00f0f2aedf797616b3f2b3c6762f5674bf03b15d (patch)
tree36cfad11882104f15245abea8eebb0524796f802 /docs/development/reviewing-patches.rst
parente202a049ff6e3bcd5ba3b3c95356b57982ffaa42 (diff)
parent91c776fe1d577efa67beb4ea26caf9492bae4070 (diff)
downloadcryptography-00f0f2aedf797616b3f2b3c6762f5674bf03b15d.tar.gz
cryptography-00f0f2aedf797616b3f2b3c6762f5674bf03b15d.tar.bz2
cryptography-00f0f2aedf797616b3f2b3c6762f5674bf03b15d.zip
Merge pull request #594 from reaperhulk/reorganize-contributing
Reorganize contributing into development section
Diffstat (limited to 'docs/development/reviewing-patches.rst')
-rw-r--r--docs/development/reviewing-patches.rst56
1 files changed, 56 insertions, 0 deletions
diff --git a/docs/development/reviewing-patches.rst b/docs/development/reviewing-patches.rst
new file mode 100644
index 00000000..302c998e
--- /dev/null
+++ b/docs/development/reviewing-patches.rst
@@ -0,0 +1,56 @@
+Reviewing/Merging Patches
+=========================
+
+Everyone is encouraged to review open pull requests. When reviewing a patch try
+to keep each of these concepts in mind:
+
+Architecture
+------------
+
+* Is the proposed change being made in the correct place? Is it a fix in a
+ backend when it should be in the primitives?
+
+Intent
+------
+
+* What is the change being proposed?
+* Do we want this feature or is the bug they're fixing really a bug?
+
+Implementation
+--------------
+
+* Does the change do what the author claims?
+* Are there sufficient tests?
+* Has it been documented?
+* Will this change introduce new bugs?
+
+Grammar/Style
+-------------
+
+These are small things that are not caught by the automated style checkers.
+
+* Does a variable need a better name?
+* Should this be a keyword argument?
+
+Merge Requirements
+------------------
+
+Because cryptography is so complex, and the implications of getting it wrong so
+devastating, ``cryptography`` has a strict merge policy for committers:
+
+* Patches must *never* be pushed directly to ``master``, all changes (even the
+ most trivial typo fixes!) must be submitted as a pull request.
+* A committer may *never* merge their own pull request, a second party must
+ merge their changes. If multiple people work on a pull request, it must be
+ merged by someone who did not work on it.
+* A patch that breaks tests, or introduces regressions by changing or removing
+ existing tests should not be merged. Tests must always be passing on
+ ``master``.
+* If somehow the tests get into a failing state on ``master`` (such as by a
+ backwards incompatible release of a dependency) no pull requests may be
+ merged until this is rectified.
+* All merged patches must have 100% test coverage.
+
+The purpose of these policies is to minimize the chances we merge a change
+that jeopardizes our users' security.
+