{
 "cells": [
  {
   "cell_type": "markdown",
   "metadata": {},
   "source": [
    "# Reviewer Guidelines"
   ]
  },
  {
   "cell_type": "markdown",
   "metadata": {},
   "source": [
    "## How to review a pull request\n",
    "\n",
    "- Be friendly\n",
    "- Read and follow the [**Reviewer Checklist**](#reviewer-checklist)\n",
    "- Decide how much time you can spare and the detail you can work in. Tell the author!\n",
    "- Use the comment/chat functionality within GitHub's pull requests - it's useful to have an archive of discussions and the decisions made.\n",
    "- Fix the big things first! If there are more important issues, not every style guide has to be stuck to,\\\n",
    "  not every slight increase in speed needs to be pointed out, and test coverage doesn't have to be 100%.\n",
    "- Make it clear when a change is optional, or is a matter of opinion"
   ]
  },
  {
   "cell_type": "markdown",
   "metadata": {},
   "source": [
    "At a minimum\n",
    "- Make sure unit and integration tests are passing.\n",
    "- (For complete modules) Run the tutorial on your local machine and check it does what it says it does\n",
    "- Check everything is fully documented"
   ]
  },
  {
   "cell_type": "markdown",
   "metadata": {},
   "source": [
    "At least one reviewer needs to\n",
    "- Review all the changes in the pull request. Read what it's supposed to do, check it does that, and make sure the logic is sound.\n",
    "- Check that the code follows the CLIMADA style guidelines \n",
    "- [CLIMADA coding conventions](../development/Guide_CLIMADA_conventions.ipynb) \n",
    "- [Python Dos and Don't](../development/Guide_PythonDos-n-Donts.ipynb) \n",
    "- [Python performance tips and best practice for CLIMADA developers](../development/Guide_Py_Performance.ipynb)  \n",
    "- If the code is implementing an algorithm it should be referenced in the documentation. Check it's implemented correctly.\n",
    "- Try to think of edge cases and ways the code could break. See if there's appropriate error handling in cases where the function might behave unexpectedly.\n",
    "- (Optional) suggest easy ways to speed up the code, and more elegant ways to achieve the same goal.\n",
    "\n",
    "There are a few ways to suggest changes\n",
    "- As questions and comments on the pull request page\n",
    "- As code suggestions (max a few lines) in the code review tools on GitHub. The author can then approve and commit the changes from GitHub pull request page. This is great for typos and little stylistic changes.\n",
    "- If you decide to help the author with changes, you can either push them to the same branch, or create a new branch and make a pull request with the changes back into the branch you're reviewing. This lets the author review it and merge."
   ]
  },
  {
   "cell_type": "markdown",
   "metadata": {},
   "source": [
    "## Setup the environment"
   ]
  },
  {
   "cell_type": "markdown",
   "metadata": {},
   "source": [
    "In order to make your review, you will have to have climada installed from source (see [here](../getting-started/install.rst#install-advanced)), and switch to the branch you are reviewing (see [here](../getting-started/install.rst#change-branch))\n",
    "\n",
    "Creating a new python environment is often not necessary (*e.g.*, for minor feature branch that do not change the dependencies you can probably use a generic `develop` environment where you installed climada in editable mode), but can help in some cases (for instance changes in dependencies), to do so:\n",
    "\n",
    "Here is a generic set of instructions which should always work, assuming you already cloned the climada repository, and are at the root of that folder:\n",
    "\n",
    "```\n",
    "git fetch && git checkout <branch to review>\n",
    "mamba create -n <choose a name> # restrict python version here with \"python==3.x.*\"\n",
    "mamba env update -n <same name> -f requirements/env_climada.yml\n",
    "mamba activate <same name>\n",
    "pip install -e ./\n",
    "```\n"
   ]
  },
  {
   "cell_type": "markdown",
   "metadata": {},
   "source": [
    "<a id='reviewer_checklist'></a>\n",
    "\n",
    "## Reviewer Checklist\n",
    "\n",
    "-  The code must be readable without extra effort from your part. The\n",
    "   code should be easily readable (for infos e.g.\n",
    "   [here](https://treyhunner.com/2017/07/craft-your-python-like-poetry/?__s=jf8h91lx6zhl7vv6o9jo))\n",
    "-  Include references to the used algorithms in the docstring\n",
    "-  If the algorithm is new, please include a description in the\n",
    "   docstring, or be sure to include a reference as soon as you publish\n",
    "   the work\n",
    "-  Variable names should be chosen to be clear. Avoid\n",
    "   ``item, element, var, list, data`` etc... A good variable name makes\n",
    "   it immediately clear what it contains.\n",
    "-  Avoid as much as possible hard-coded indices for list (no\n",
    "   ``x = l[0], y = l[1]``). Rather, use tuple unpacking (see\n",
    "   [here](https://treyhunner.com/2018/03/tuple-unpacking-improves-python-code-readability/)).\n",
    "   Note that tuple unpacking can also be used to update variables. For\n",
    "   example, the Fibonacci sequence next number pair can be written as\n",
    "   ``n1, n2 = n2, n1+n2``.\n",
    "-  Do not use\n",
    "   [mutable](https://www.geeksforgeeks.org/mutable-vs-immutable-objects-in-python/)\n",
    "   (lists, dictionaries, ...) as [default values for functions and\n",
    "   methods](https://stackoverflow.com/questions/1132941/least-astonishment-and-the-mutable-default-argument).\n",
    "   Do not write: \n",
    "    ```  \n",
    "    def function(default=[]):\n",
    "    ```  \n",
    "    \n",
    "    but use\n",
    "    ```   \t\n",
    "    def function(default=None):\n",
    "        if default is None: default=[]\n",
    "    ```  \n",
    "\n",
    "-  Use pythonic loops, [list\n",
    "   comprehensions](https://treyhunner.com/2015/12/python-list-comprehensions-now-in-color/)\n",
    "-  Make sure the unit tests are testing all the lines of the code. Do not\n",
    "   only check for working cases, but also the most common wrong use\n",
    "   cases.\n",
    "-  Check the docstrings (Do they follow the [Numpydoc conventions](https://numpydoc.readthedocs.io/en/latest/format.html), is everything clearly explained, are the default\n",
    "   values given and is it clear why they are set to this value)\n",
    "-  Keep the code simple. Avoid using complex Python functionalities whose use\n",
    "   is opaque to non-expert developers unless necessary. For example, the\n",
    "   ``@staticmethod`` decorator should only be used if really \n",
    "   necessary. Another example, for counting the dictionary\n",
    "   ``colors = ['red', 'green', 'red', 'blue', 'green', 'red']``,\n",
    "   version:\n",
    "   ```   \t\n",
    "   d = {}\n",
    "   for color in colors:\n",
    "            d[color] = d.get(color, 0) + 1 \n",
    "   ```   \t\n",
    "   is perfectly fine, no need to complicate it to a maybe more pythonic\n",
    "   version\n",
    "    ```   \t\n",
    "   \td = collections.defaultdict(int)\n",
    "    for color in colors:\n",
    "        d[color] += 1\n",
    "    ```   \t\n",
    "\n",
    "-  Did the code writer perform a static code analysis? Does the code\n",
    "   respect Pep8 (see also the [pylint config file](https://github.com/CLIMADA-project/climada_python/blob/main/.pylintrc/))?\n",
    "-  Did the code writer perform a profiling and checked that there are no\n",
    "   obviously inefficient (computation time-wise and memory-wise) parts in\n",
    "   the code?"
   ]
  },
  {
   "cell_type": "code",
   "execution_count": null,
   "metadata": {},
   "outputs": [],
   "source": []
  }
 ],
 "metadata": {
  "kernelspec": {
   "display_name": "",
   "name": ""
  },
  "language_info": {
   "name": "python"
  }
 },
 "nbformat": 4,
 "nbformat_minor": 4
}