1Reviewing and merging patches 2============================= 3 4Everyone is encouraged to review open pull requests. We only ask that you try 5and think carefully, ask questions and are `excellent to one another`_. Code 6review is our opportunity to share knowledge, design ideas and make friends. 7 8When reviewing a patch try to keep each of these concepts in mind: 9 10Architecture 11------------ 12 13* Is the proposed change being made in the correct place? Is it a fix in a 14 backend when it should be in the primitives? 15 16Intent 17------ 18 19* What is the change being proposed? 20* Do we want this feature or is the bug they're fixing really a bug? 21 22Implementation 23-------------- 24 25* Does the change do what the author claims? 26* Are there sufficient tests? 27* Has it been documented? 28* Will this change introduce new bugs? 29 30Grammar and style 31----------------- 32 33These are small things that are not caught by the automated style checkers. 34 35* Does a variable need a better name? 36* Should this be a keyword argument? 37 38Merge requirements 39------------------ 40 41Because cryptography is so complex, and the implications of getting it wrong so 42devastating, ``cryptography`` has a strict merge policy for committers: 43 44* Patches must *never* be pushed directly to ``master``, all changes (even the 45 most trivial typo fixes!) must be submitted as a pull request. 46* A committer may *never* merge their own pull request, a second party must 47 merge their changes. If multiple people work on a pull request, it must be 48 merged by someone who did not work on it. 49* A patch that breaks tests, or introduces regressions by changing or removing 50 existing tests should not be merged. Tests must always be passing on 51 ``master``. 52* If somehow the tests get into a failing state on ``master`` (such as by a 53 backwards incompatible release of a dependency) no pull requests may be 54 merged until this is rectified. 55* All merged patches must have 100% test coverage. 56 57The purpose of these policies is to minimize the chances we merge a change 58that jeopardizes our users' security. 59 60.. _`excellent to one another`: https://speakerdeck.com/ohrite/better-code-review 61