diff options
author | Alex Gaynor <alex.gaynor@gmail.com> | 2014-02-11 21:32:11 -0800 |
---|---|---|
committer | Alex Gaynor <alex.gaynor@gmail.com> | 2014-02-11 21:32:11 -0800 |
commit | 00f0f2aedf797616b3f2b3c6762f5674bf03b15d (patch) | |
tree | 36cfad11882104f15245abea8eebb0524796f802 /docs/development/reviewing-patches.rst | |
parent | e202a049ff6e3bcd5ba3b3c95356b57982ffaa42 (diff) | |
parent | 91c776fe1d577efa67beb4ea26caf9492bae4070 (diff) | |
download | cryptography-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.rst | 56 |
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. + |