| Alex Stapleton | a678ae0 | 2014-03-29 13:26:10 +0000 | [diff] [blame] | 1 | Reviewing and merging patches |
| 2 | ============================= |
| Paul Kehrer | 0839aa8 | 2014-02-11 22:36:51 -0600 | [diff] [blame] | 3 | |
| Alex Stapleton | a678ae0 | 2014-03-29 13:26:10 +0000 | [diff] [blame] | 4 | Everyone is encouraged to review open pull requests. We only ask that you try |
| 5 | and think carefully, ask questions and are `excellent to one another`_. Code |
| 6 | review is our opportunity to share knowledge, design ideas and make friends. |
| 7 | |
| 8 | When reviewing a patch try to keep each of these concepts in mind: |
| Paul Kehrer | 0839aa8 | 2014-02-11 22:36:51 -0600 | [diff] [blame] | 9 | |
| 10 | Architecture |
| 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 | |
| 16 | Intent |
| 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 | |
| 22 | Implementation |
| 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 | |
| Alex Stapleton | a678ae0 | 2014-03-29 13:26:10 +0000 | [diff] [blame] | 30 | Grammar and style |
| 31 | ----------------- |
| Paul Kehrer | 0839aa8 | 2014-02-11 22:36:51 -0600 | [diff] [blame] | 32 | |
| 33 | These 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? |
| Paul Kehrer | 91c776f | 2014-02-11 23:08:47 -0600 | [diff] [blame] | 37 | |
| Alex Stapleton | c5fffd3 | 2014-03-18 15:29:00 +0000 | [diff] [blame] | 38 | Merge requirements |
| Paul Kehrer | 91c776f | 2014-02-11 23:08:47 -0600 | [diff] [blame] | 39 | ------------------ |
| 40 | |
| 41 | Because cryptography is so complex, and the implications of getting it wrong so |
| 42 | devastating, ``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 | |
| 57 | The purpose of these policies is to minimize the chances we merge a change |
| 58 | that jeopardizes our users' security. |
| 59 | |
| Alex Stapleton | a678ae0 | 2014-03-29 13:26:10 +0000 | [diff] [blame] | 60 | .. _`excellent to one another`: https://speakerdeck.com/ohrite/better-code-review |