Review of pull requests

In the ESMValTool community we use pull request reviews to ensure all code and documentation contributions are of good quality. An introduction to code reviews can be found in The Turing Way, including why code reviews are important and advice on how to have constructive reviews.

Most pull requests will need two reviews before they can be merged. First a technical review takes place and then a scientific review. Once both reviewers have approved a pull request, it can be merged. These three steps are described in more detail below. If a pull request contains only technical changes, e.g. a pull request that corrects some spelling errors in the documentation or a pull request that fixes some installation problem, a scientific review is not needed.

If you are a regular contributor, please try to review a bit more than two other pull requests for every pull request you create yourself, to make sure that each pull request gets the attention it deserves.

1. Technical review

Technical reviews are done by the technical review team. This team consists of regular contributors that have a strong interest and experience in software engineering.

Technical reviewers use the technical checklist from the pull request template to make sure the pull request follows the standards we would like to uphold as a community. The technical reviewer also keeps an eye on the design and checks that no major design changes are made without the approval from the technical lead development team. If needed, the technical reviewer can help with programming questions, design questions, and other technical issues.

The technical review team can be contacted by writing @ESMValGroup/tech-reviewers in a comment on an issue or pull request on GitHub.

2. Scientific review

Scientific reviews are done by the scientific review team. This team consists of contributors that have a strong interest and experience in climate science or related domains.

Scientific reviewers use the scientific checklist from the pull request template to make sure the pull request follows the standards we would like to uphold as a community.

The scientific review team can be contacted by writing @ESMValGroup/science-reviewers in a comment on an issue or pull request on GitHub.

3. Merge

Pull requests are merged by the @ESMValGroup/esmvaltool-coreteam. Specifically, pull requests containing a CMORizer script can only be merged by @remi-kazeroni, who will then add the CMORized data to the OBS data pool at DKRZ and CEDA-Jasmin. The team member who does the merge first checks that both the technical and scientific reviewer approved the pull request and that the reviews were conducted thoroughly. He or she looks at the list of files that were changed in the pull request and checks that all relevant checkboxes from the checklist in the pull request template have been added and ticked.

The core development team can be contacted by writing @ESMValGroup/esmvaltool-coreteam in a comment on an issue or pull request on GitHub.

Frequently asked questions

How do I request a review of my pull request?

If you know a suitable reviewer, e.g. because your pull request fixes an issue that they opened or they are otherwise interested in the work you are contributing, you can ask them for a review by clicking the cogwheel next to ‘Reviewers’ on the pull request ‘Conversation’ tab and clicking on that person. When changing code, it is a good idea to ask the original authors of that code for a review. An easy way to find out who previously worked on a particular piece of code is to use git blame. GitHub will also suggest reviewers based on who previously worked on the files changed in a pull request. Every recipe has a maintainer and authors listed in the recipe, it is a good idea to ask these people for a review.

If there is no obvious reviewer, you can attract the attention of the relevant team of reviewers by writing to @ESMValGroup/tech-reviewers or @ESMValGroup/science-reviewers in a comment on your pull request.

How do I optimize for a fast review?

When authoring a pull request, please keep in mind that it is easier and faster to review a pull request that does not contain many changes. Try to add one new feature per pull request and change only a few files. For the ESMValTool repository, try to limit changes to a few hundred lines of code and new diagnostics to not much more than a thousand lines of code. For the ESMValCore repository, a pull request should ideally change no more than about a hundred lines of existing code, though adding more lines for unit tests and documentation is fine.

If you are a regular contributor, make sure you regularly review other people’s pull requests, that way they will be more inclined to return the favor by reviewing your pull request.

How do I find a pull request to review?

Please pick pull requests to review yourself based on your interest or expertise. We try to be self organizing, so there is no central authority that will assign you to review anything. If someone knows you have expertise on a certain topic, they might request your review on a pull request though. If your review is requested, please try to respond within a few days if at all possible. If you do not have the time to review the pull request, notify the author and try to find a replacement reviewer.

How do I actually do a review?

To do a review, go to the pull request on GitHub, the list of all pull requests is available here https://github.com/ESMValGroup/ESMValCore/pulls for the ESMValCore and here https://github.com/ESMValGroup/ESMValTool/pulls for the ESMValTool, click the pull request you would like to review. The top comment should contain (a selection of) the checklist available in the pull request template. If it is not there, copy the relevant items from the pull request template. Which items from the checklist are relevant, depends on which files are changed in the pull request. A list of items to check with brief explanations is given in section Checklists for reviewing a pull request. The items are grouped by technical and scientific review. To see which files have changed, click the tab ‘Files changed’. To comment on specific lines of code or documentation, click the ‘plus’ icon next to a line of code and write your comment. When you are done reviewing, use the ‘Review changes’ button in the top right corner to comment on, request changes to, or approve the pull request.

What if the author and reviewer disagree?

When the author and the reviewer of a pull request have difficulty agreeing on what needs to be done before the pull request can be approved, it is usually both more pleasant and more efficient to schedule a meeting or co-working session, for example using Google meet or Jitsi meet.

When reviewing a pull request, try to refrain from making changes to the pull request yourself, unless the author specifically agrees to those changes, as this could potentially be perceived as offensive.

If talking about the pull requests in a meeting still does not resolve the disagreement, ask a member of the @ESMValGroup/esmvaltool-coreteam for their opinion and try to find a solution.

Checklists for reviewing a pull request

Below are general checklists for doing technical and scientific reviews including brief descriptions of the tasks to do. Reviewing CMORizer scripts consists mostly of technical tasks but differs slightly from the technical review tasks and is therefore listed in a third section below.

Technical reviews

Documentation

Check that the scientific documentation of the new diagnostic has been added to the user’s guide:

  • A file (./doc/sphinx/source/recipes/recipe_<diagnostic>.rst) exists

  • New documentation is included in ./doc/sphinx/source/recipes/index.rst

  • Documentation follows template (./doc/sphinx/source/recipes/recipe_template.rst.template)

  • Description of configuration options

  • Description of variables

  • Valid image files

  • Resolution of image files (~150 dpi is usually enough; file size should be kept small)

Recipe

Check yaml syntax and that new recipe contains:

  • Documentation: description, authors, maintainer, references, projects

  • Provenance tags: themes, realms

Diagnostic script

Check that the new diagnostic script(s) meet(s) standards. This includes the following items:

  • In-code documentation (comments, docstrings)

  • Code quality (e.g. no hardcoded pathnames)

  • No Codacy errors reported

  • Re-use of existing functions whenever possible

  • Provenance implemented

Run recipe

Make sure new diagnostic(s) is working by running the ESMValTool with the recipe.

Check output of diagnostic

After successfully running the new recipe, check that:

  • Netcdf output has been written

  • Output contains (some) valid values (e.g. not only nan or zeros)

  • Provenance information has been written

Check automated tests

Check for errors reported by automated tests

  • Codacy

  • CircleCI

  • Documentation build

Scientific reviews

Documentation added to user’s guide

Check that the scientific documentation of the new diagnostic (./doc/sphinx/source/recipes/recipe_<diagnostic>.rst):

  • Meets scientific documentation standard and

  • Contains brief description of method

  • Contains references

  • Check for typos / broken text

  • Documentation is complete and written in an understandable language

  • References are complete

Recipe

Check that new recipe contains valid:

  • Documentation: description, references

  • Provenance tags: themes, realms

Diagnostic script

Check that the new diagnostic script(s) meet(s) scientific standards. This can include the following items:

  • Clear and understandable in-code documentation including brief description of diagnostic

  • References

  • Method / equations match reference(s) given

Run recipe

Make sure new diagnostic(s) is working by running the ESMValTool.

Check output of diagnostic

After successfully running the new recipe, check that:

  • Output contains (some) valid values (e.g. not only nan or zeros)

  • If applicable, check plots and compare with corresponding plots in the paper(s) cited

CMORizer scripts

Reviewing CMORizer scripts differs slightly from reviewing technical changes or scientific reviews of new diagnostics. A review typically contains mostly technical aspects given in the checklist below.

Dataset description

Check that new dataset has been added to the table of observations defined in the ESMValTool guide user’s guide in section “Obtaining input data” (./doc/sphinx/source/input.rst).

BibTeX info file

Check that a BibTeX file (i.e. <dataset>.bibtex) defining the reference(s) for the new dataset has been created in ./esmvaltool/references/.

recipe_check_obs.yml

Check that new dataset has been added to the testing recipe ./esmvaltool/recipes/examples/recipe_check_obs.yml

CMORizer script

Check that the new CMORizer script (./esmvaltool/cmorizers/obs/cmorize_obs_<dataset>.{py,ncl,r}) meets standards. This includes the following items:

  • In-code documentation (header) contains

    1. Download instructions

    2. Reference(s)

  • Code quality checks

    1. Code quality (e.g. no hardcoded pathnames)

    2. No Codacy errors reported

Config file

If present, check config file <dataset>.yml in ./esmvaltool/cmorizers/obs/cmor_config/.

Run CMORizer

Make sure CMORizer is working by running cmorize_obs -c <config-file> -o <dataset>

Check output of CMORizer

After successfully running the new CMORizer, check that:

  • Output contains (some) valid values (e.g. not only nan or zeros)

  • Metadata is defined properly

Run ./esmvaltool/recipes/examples/recipe_check_obs.yml for new dataset.

RAW data

Contact person in charge of ESMValTool data pool (@remi-kazeroni) and request to copy RAW data to RAWOBS/Tier2 (Tier3).

CMORized data

Contact person in charge of ESMValTool data pool (@remi-kazeroni) and request to

  • Merge the pull request

  • Copy CMORized dataset to OBS/Tier2 (Tier3)

  • Set file access rights for new dataset

After merging a pull request

After merging a pull request successfully, the @ESMValGroup/esmvaltool-coreteam member who merged the pull request will:

  • Close related issue if existent