61 lines
2.1 KiB
ReStructuredText
61 lines
2.1 KiB
ReStructuredText
Reviewing and merging patches
|
|
=============================
|
|
|
|
Everyone is encouraged to review open pull requests. We only ask that you try
|
|
and think carefully, ask questions and are `excellent to one another`_. Code
|
|
review is our opportunity to share knowledge, design ideas and make friends.
|
|
|
|
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 and 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.
|
|
|
|
.. _`excellent to one another`: https://speakerdeck.com/ohrite/better-code-review
|