[#1462] Add guidelines for reviewing pull requests

doc/contributing/reviewing.rst

@@ -2,14 +2,84 @@
 Reviewing and merging a pull request
 ====================================
 
-.. todo::
+Of course it's not possible to give an exact recipe for reviewing a pull
+request, you simply have to assess the code and decide whether you're happy
+with it. Nonetheless, here is an incomplete list of things to look for:
+
+- Does the pull request contain one logically separate piece of work
+  (e.g. one new feature, bug fix, etc. per pull request)?
+
+- Does the pull request follow the guidelines for
+  :doc:`writing commit messages <commit-messages>`?
+
+- Is the branch up to date - have the latest commits from master been pulled
+  into the branch?
+
+- Does the pull request contain new or updated tests for any new or updated
+  code, and do the tests follow
+  :doc:`CKAN's testing coding standards <testing>`?
+
+- Do all the CKAN tests pass, on the new branch?
+
+- Does the pull request contain new or updated docs for any new or updated
+  features, and do the docs follow
+  :doc:`CKAN's documentation guidelines <documentation>`?
+
+- Does the new code follow CKAN's code architecture and the various coding
+  standards for Python, JavaScript, etc.?
+
+- If the new code contains changes to the database schema, does it have a
+  :doc:`database migration <database-migrations>`?
+
+- Does the code contain any changes that break backwards-incompatibility?
+  If so, is the breakage necessary or do the benefits of the change justify the
+  breakage? Have the breaking changes been added to the :doc:`changelog
+  </changelog>`?
+
+  Backwards-compability needs to be considered when making changes that break
+  the interfaces that CKAN provides to third-party code, including API clients,
+  plugins and themes.
+
+  In general, any code that's documented in the reference sections of the
+  :doc:`API </api>`, :doc:`extensions </extensions/index>` or :doc:`theming
+  </theming/index>` needs to be considered. For example this includes changes
+  to the API actions, the plugin interfaces or plugins toolkit, the converter
+  and validator functions (which are used by plugins), the custom Jinja2 tags
+  and variables available to Jinja templates, the template helper functions,
+  the core template files and their blocks, the sandbox available to JavaScript
+  modules (including custom jQuery plugins and the JavaScript CKAN API client),
+  etc.
+
+- Does the new code add any dependencies to CKAN (e.g. new third-party Python
+  modules imported)? If so, is the new dependency justified and has it been
+  added following the right process? See :doc:`upgrading-dependencies`.
 
-   Put some more detailed docs here about how to review a CKAN pull request.
 
 ----------------------
 Merging a pull request
 ----------------------
 
-If you're reviewing a pull request for CKAN, when merging a branch into master:
+Once you've reviewed a pull request and you're happy with it, you need to
+merge it into the master branch. You should do this using the ``--no-ff``
+option in the ``git merge`` command. For example::
+
+ git checkout feature-branch
+ git pull origin feature-branch
+ git checkout master
+ git pull origin master
+ git merge --no-ff feature-branch
+ git push origin master
+
+Before doing the ``git push``, it's a good idea to check that all the tests are
+passing on your master branch (if the latest commits from master have already
+been pulled into the feature branch on github, then it may be enough to check
+that all tests passed for the latest commit on this branch on
+`Travis <https://travis-ci.org/okfn/ckan>`_).
+
+Also before doing the ``git push``, it's a good idea to use ``git log`` and/or
+``git diff`` to check the difference between your local master branch and the
+remote master branch, to make sure you only push the changes you intend to
+push::
 
-- Use the ``--no-ff`` option in the ``git merge`` command
+ git log ...origin/master
+ git diff ..origin/master