From 9c3cf45ff351da87f6e15811099913300c6b465e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sarah=20H=C3=BClsen?= <49907095+sarah-hlsn@users.noreply.github.com> Date: Wed, 1 Nov 2023 09:46:42 +0100 Subject: [PATCH] move reviewer guide and checklist to new file --- doc/guide/Guide_Git_Development.ipynb | 54 -------------- ...wer_Checklist.ipynb => Guide_Review.ipynb} | 70 ++++++++++++++----- 2 files changed, 52 insertions(+), 72 deletions(-) rename doc/guide/{Guide_Reviewer_Checklist.ipynb => Guide_Review.ipynb} (58%) diff --git a/doc/guide/Guide_Git_Development.ipynb b/doc/guide/Guide_Git_Development.ipynb index 3be2fd201..05f5b5c82 100644 --- a/doc/guide/Guide_Git_Development.ipynb +++ b/doc/guide/Guide_Git_Development.ipynb @@ -608,60 +608,6 @@ "12) Update the `develop` branch on your local machine." ] }, - { - "cell_type": "markdown", - "metadata": { - "slideshow": { - "slide_type": "slide" - } - }, - "source": [ - "### How to review a pull request\n", - "\n", - "- Be friendly\n", - "- Read and follow the [**Reviewer Checklist**](../guide/Guide_Reviewer_Checklist.ipynb)\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": { - "slideshow": { - "slide_type": "subslide" - } - }, - "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": { - "slideshow": { - "slide_type": "subslide" - } - }, - "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 `#TODO: link`\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": { diff --git a/doc/guide/Guide_Reviewer_Checklist.ipynb b/doc/guide/Guide_Review.ipynb similarity index 58% rename from doc/guide/Guide_Reviewer_Checklist.ipynb rename to doc/guide/Guide_Review.ipynb index 046075775..9d9a39d5c 100644 --- a/doc/guide/Guide_Reviewer_Checklist.ipynb +++ b/doc/guide/Guide_Review.ipynb @@ -2,10 +2,58 @@ "cells": [ { "cell_type": "markdown", - "id": "commercial-participant", "metadata": {}, "source": [ - "# Reviewer Checklist\n", + "# Reviewer Guidelines" + ] + }, + { + "cell_type": "markdown", + "metadata": {}, + "source": [ + "## How to review a pull request\n", + "\n", + "- Be friendly\n", + "- Read and follow the [**Reviewer Checklist**](../guide/Guide_Reviewer_Checklist.ipynb)\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 `#TODO: link`\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": [ + "## 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", @@ -73,24 +121,10 @@ } ], "metadata": { - "kernelspec": { - "display_name": "Python 3 (ipykernel)", - "language": "python", - "name": "python3" - }, "language_info": { - "codemirror_mode": { - "name": "ipython", - "version": 3 - }, - "file_extension": ".py", - "mimetype": "text/x-python", - "name": "python", - "nbconvert_exporter": "python", - "pygments_lexer": "ipython3", - "version": "3.8.12" + "name": "python" } }, "nbformat": 4, - "nbformat_minor": 5 + "nbformat_minor": 2 }