Reviewing pull requests

This guide aims to cover the basic steps you should go through when reviewing any pull request to snoplusuk/echidna. Follow these four simple steps when reviewing a pull request, will ensure that the development version (and any tagged versions released from the development version) are always accurate, well documented and usable.

Set-up for review

This guide assumes you already have the prerequisites for running echidna e.g. all requirements installed form requirements file and a copy of the code.

You may wish to create a separate clone of echidna for reviewing code, so that untracked files you are developing don't confuse things.

$ git clone [email protected]:snoplusuk/echidna.git echidna-pr

Also you should ensure you have the GitHub pages sub repository installed in docs/html so you can update the online documentation once the pull request is merged. An guide on setting this up is available here.

The final thing you need is a copy of the code you are reviewing. If you don't already have a remote set-up for the person who's code you are reviewing, set one up!

$ git remote add <remote-name> [email protected]:username/echidna.git

Then checkout their pull requst:

$ git fetch <remote-name>
$ git checkout <remote-name>/<pull-request-branch>

where pull-request-branch is the requested branch to merge into snoplus:master in the pull request.

You're now all set-up to start reviewing.

Step 1: Documentation

From its creation, we have worked really hard with echidna, not only to make sure it is well documented, but that it is easy to add documentation and keep it up-to-date. And equally that the documentation is easy to access - there is no point having great documentation, if no-one knows how to access it.

Hence the first step in the review process should be build the documentation and read through the relevant parts. By doing this, not only will you check the documentation for future users, but you will familiarise yourself with the part(s) of the code you are reviewing. This is especially useful if it is a completely new class/module or part you haven't used before in your own work.

Build the documentation in the usual way: (from the base directory)

$ sphinx-apidoc -f -H echidna -o docs echidna/
$ cd docs
$ make html

As of #59 the html output goes to docs/html/docs so we can display the latest docs online using GitHub Pages.

• Does the documentation build correctly? If it gives any errors notify the reviewee. Ideally there should be no warnings either. Make sure it is not an untracked file that you are developing that is causing the errors/warnings -- this is why it can be a good idea to set up a separate clone of echidna for reviewing.

Possibly easier looking back at the diff/code:

• For each/any new module, function, class and class method check that it has been documented.

• Is the documentation complete? See this guide.

• Equally we don't want over-documentation. An example, consider a getter method that just returns the value of a class member _chi_squared at some point in the code:

  def get_chi_squared(self):
            return self._chi_squared
  

  Since all this function does is return the value of a class member, it is probably overkill to have a description and Returns heading in the docstring. It would probably be sufficient to write:

  def get_chi_squared(self):
      """
        Returns:
          float. Current value of :attr:`_chi_squareds`
      """
      return self._chi_squared
  

Switching back to the built documentation, check:

• Is it built correctly? Correct formatting for lists of args, code snippets, return values, references to other classes/methods (echidna or external)?
• Spelling and grammar -- we all make mistakes!
• Do you actually understand what is happening in the module/class/method/function now? If not the documentation hasn't really done it's job?

Step 2: Testing

The unittests in are included in echidna/test. One of the main purposes of unittesting is to look at small parts of the code, check that they are working in the way that is expected and that they continue to work as expected when code is changed or new code is added.

Whenever we have a pull request, either existing code is being changed or new code added, so it makes sense to run the unittests and check that all tests are passed.

Run the tests in the usual way: (from the base directory):

$ python -m unittest discover echidna/test/

• Do all the tests pass? Pay particular attention to any new tests, tests that relate to the part of the code that has been modified or tests that have been modified as part of the pull request.
  • If there are failures or errors let the reviewee know.
• Should any further tests be added? This is probably the hardest part of the reviewing process to document and contemplate whilst reviewing. But If you can think of an obvious case where something could go wrong that is not tested, then an appropriate test should probably be added.

Run any example scripts directly relating to changes. These are in echidna/scripts or may be found in the code block if __name__ == "__main__" in modules.

• Do these run without Exception? Do they behave as expected?
• Do they need to be updated in light of the changes?

Step 3: coding style and standards

We decided that, like any modern python package, echidna should strive for pep8 compliance -- at the very least. For more details on pep8 and why our code should try to follow its standards, see [this guide].

Remember though that pep8 is only a guide, there are cases where following pep8 guidelines explicitly may reduce readability in a piece of code. You should also be aware that people do have their own coding style, and what is more readable to one person might not necessarily be readable to another.

For these reasons, while you should certainly check the code you're reviewing for pep8 compliance and report back any errors or warnings, merging a pull request should not be refused solely because of pep8 errors.

Check the code for pep8 compliance by:

$ pep8 module_to_check.py

Any errors will be reported.

• Were there any errors? Do they cause major issues for readability? Let the reviewee know.

Optional extension: pylint

You could also try checking the code with pylint!

$ pylint module_to_check.py

This will give you a very detailed report on the code. As well as checking basic python syntax and pep8, it will report on violations of naming conventions and other stylistic points (e.g. too many/few public methods), missing docstrings, unused imports, to name but a few. It needs some agreement on configuration, if we actually want to use it as standard for echidna, but it is interesting to run it out-the-box, just to see how the code fairs.

Actually checking the code

If you've followed all the steps so far, you should now be convinced that the code is well documented, fully tested and readable. What we haven't checked yet is the source code itself. There are some things that can't be checked by built-in tools.

Read through the code (on the GitHub diff or in an editor):

• Does the code follow correct python syntax? (pep8 and pylint should tell you this to some extent, but best to check)
• Does the algorithm make sense? You can write something in perfect python based on a completely useless underlying algorithm; this is not going to be particularly useful to anyone.
• Are there any physics errors? At the end of the day we are trying to do physics here, so we should also catch any bits of code that are unphysical!

Step 4: Merging!

By now the code should be ready to merge! If all is OK you should be able to merge it on GitHub by clicking on the "merge" button. It is usually fine to keep the default merge commit message GitHub gives you.

In the unlikely event that you can't merge automatically, you'll have to do it via the command line and resolve any conflicts.

1. Switch to a branch that is fully up-to-date with snoplusuk/echidna (if you are using a separate clone for reviews, the master branch here will be fine.)

2. Run:

    $ git merge reviewee's_remote/code_you_just_reviewed
   

3. Follow the instructions on resolving merge conflicts

Once the code is successfully merged, switch back to the branch you were on to review the code and commit the changes to the documentation.

$ cd docs/html
$ git status 
# Should show changes in docs/ directory unstaged for commit
$ git commit -a
# Give the commit a sensible message
$ git push origin gh-pages

Try to reference the pull request in the commit message, so we know what the documentation change refers to if we ever have to revert a pull request, e.g. "Updates online docs for #60"

That's it, you've successfully reviewed the pull request!

A few final points

• Why not check out this demo of me reviewing PR #76. It doesn't work perfectly, but is still quite cool! You may wish too watch it at 5x speed because of my slow typing and occasional mistakes!
• It is a good idea to read through this both when reviewing and as you submit a pull request -- so you remind yourself exactly what the reviewer will be looking out for and hopefully make their life a bit easier.
• Following this order and using the tools suggested, should make it relatively easy to complete reviews and mean that you spend as little time as possible just staring at someone else's code trying to work out what they've done.
• Try to start the review process within a week of being assigned
• Keep the reviewee informed. Post comments/errors on GitHub, which they can resolve and add further commits. Consider posting a comment or adding a label when you finish a review step.
• Reviewee's are advised to strongly consider making all the changes suggested by the reviewer, but remember some coding style or algorithmic choices are down to personal preference and should not be reason to refuse a merge.

Happy reviewing!