From b63ab683736201d4ef9f85d8d1319ee66eaf80de Mon Sep 17 00:00:00 2001 From: Steven Maude Date: Thu, 24 Mar 2022 17:19:42 +0000 Subject: [PATCH 01/11] Split code review content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is using the Diátaxis approaches of: * picking some documentation arbitrarily and trying to make small improvements * keeping different categories of documentation separate As written, this felt like a mixture of explanation and how-to guide, so split these out into two different pages. --- docs/code-reviews-pull-request.md | 53 +++++++++++++++++++++++++++++++ docs/code-reviews.md | 53 ------------------------------- mkdocs.yml | 4 ++- 3 files changed, 56 insertions(+), 54 deletions(-) create mode 100644 docs/code-reviews-pull-request.md diff --git a/docs/code-reviews-pull-request.md b/docs/code-reviews-pull-request.md new file mode 100644 index 000000000..1a13f0fa5 --- /dev/null +++ b/docs/code-reviews-pull-request.md @@ -0,0 +1,53 @@ +## How to ask for a review + +GitHub has a feature built into the workflow that allows easy code review with collaborators, and we would recommend +that you use this tool. GitHub has some [documentation on pull requests and code reviews](https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-request-reviews) which we recommend for background reading. + +![An example of GitHub's pull request feature.](./images/code-review-main.png) + +In this screenshot, the user is making a new pull request. In the top right-hand corner (marked by the +red box), there is a tagging function under Reviewers where you can search or select collaborators to do a code review. +Once you have tagged them and made the pull request, the person will receive a notification to their +email, and it will also come up in the pull request section of their GitHub main screen. + +![An example of a pull request description as shown on GitHub.](./images/pr-desc.png) + +It is strongly encouraged to add some information to +your request to aid the reviewer. + +## How can I write a good pull request? + +Some best practice gathered from current users of OpenSAFELY are described below: + + - Thinking about the purpose of the review can be helpful as this may +vary from review to review. For example, are you looking for a check of the implementation of the logic or methods +outlined in the protocol (in which case, linking the protocol can aid this), or are you looking for a +review of how well you document your code and if it is clear and understandable? Being transparent +will help your reviewer do a better job quicker. +- Good descriptive names for pull requests and commits help the reviewer follow the logic of the code and the changes made (see example of commits below). +- Multiple smaller pull requests are easier than one large. +- Code comments and documentations make the logic easier to follow. +- Naming the order of the files, for example, `01_Data_Extraction.py`, `02_Data_Cleaning.py`, etc. +- Allow sufficient time for the review. + +### Examples of good commit messages +![An example of a good commit message as shown on GitHub.](./images/good-pr-pic.png) + +## How can I be a good reviewer? +Some best practices gathered from current users of OpenSAFELY are described below: + +- Code review should be a collaborative conversation and don't be afraid to check assumptions or clarify intentions within the review (you can add messages to the end of the review request). +- Actionable feedback with suggestions is helpful. +- It can be helpful to highlight the levels of code review comments. For example, +if something is nice-to-have-but-not-important, you can prefix it with `Nit:`. For example, +`Nit: I think this would be cleared if you didn't use an underscore in this variable name` +- Being clear about what exactly you have reviewed or how thorough the review was - for example, being clear if you read through the code but did not run it (and therefore may miss running errors in implementation). Unit tests should catch that the code is runnable, but will not necessarily check it is correct. +- Answer specific questions in the review request. +- Avoiding jabs on coding style. +- Being clear if you do not have time to do a thorough review. + +## What actions can we take as a team to encourage code reviews? + +- Those in leadership roles should volunteer their time do code reviews, or help find code reviewers, to help embed the culture of code-reviews widely. +- New projects can have a "buddy" who can help to review the code, and means they will be familiar with the protocol and code changes from the start. + diff --git a/docs/code-reviews.md b/docs/code-reviews.md index 1d9213974..624f1d190 100644 --- a/docs/code-reviews.md +++ b/docs/code-reviews.md @@ -7,23 +7,6 @@ improvements. It is an everyday activity in the software development world and improves both quality and speed in programming. Code reviews also offer opportunities for both code author and code reviewer to learn from each other. Code reviews are standard practice within OpenSAFELY studies and the default should be to always get your code reviewed. -## How to ask for a review - -GitHub has a feature built into the workflow that allows easy code review with collaborators, and we would recommend -that you use this tool. GitHub has some [documentation on pull requests and code reviews](https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-request-reviews) which we recommend for background reading. - -![An example of GitHub's pull request feature.](./images/code-review-main.png) - -In this screenshot, the user is making a new pull request. In the top right-hand corner (marked by the -red box), there is a tagging function under Reviewers where you can search or select collaborators to do a code review. -Once you have tagged them and made the pull request, the person will receive a notification to their -email, and it will also come up in the pull request section of their GitHub main screen. - -![An example of a pull request description as shown on GitHub.](./images/pr-desc.png) - -It is strongly encouraged to add some information to -your request to aid the reviewer. - ## When to ask for code review? Code reviews should be the default. A good way of conceptualising when to ask for a review is to think about commits and branches. All your work should be done in branches and you should aim to @@ -32,39 +15,3 @@ accompanied with a code review. If you are in a hurry, and no-one has time to do code review, bear in mind that even a cursory code review is better than no code review. Merging a new branch should not be your only trigger for a review. If you are stuck or unsure what direction to take, a code review can clarify what path to take. In general, the more frequent the reviews, the better quality the code tends to be. - -## How can I write a good pull request? -Some best practice gathered from current users of OpenSAFELY are described below: - - - Thinking about the purpose of the review can be helpful as this may -vary from review to review. For example, are you looking for a check of the implementation of the logic or methods -outlined in the protocol (in which case, linking the protocol can aid this), or are you looking for a -review of how well you document your code and if it is clear and understandable? Being transparent -will help your reviewer do a better job quicker. -- Good descriptive names for pull requests and commits help the reviewer follow the logic of the code and the changes made (see example of commits below). -- Multiple smaller pull requests are easier than one large. -- Code comments and documentations make the logic easier to follow. -- Naming the order of the files, for example, `01_Data_Extraction.py`, `02_Data_Cleaning.py`, etc. -- Allow sufficient time for the review. - -### Examples of good commit messages -![An example of a good commit message as shown on GitHub.](./images/good-pr-pic.png) - -## How can I be a good reviewer? -Some best practices gathered from current users of OpenSAFELY are described below: - -- Code review should be a collaborative conversation and don't be afraid to check assumptions or clarify intentions within the review (you can add messages to the end of the review request). -- Actionable feedback with suggestions is helpful. -- It can be helpful to highlight the levels of code review comments. For example, -if something is nice-to-have-but-not-important, you can prefix it with `Nit:`. For example, -`Nit: I think this would be cleared if you didn't use an underscore in this variable name` -- Being clear about what exactly you have reviewed or how thorough the review was - for example, being clear if you read through the code but did not run it (and therefore may miss running errors in implementation). Unit tests should catch that the code is runnable, but will not necessarily check it is correct. -- Answer specific questions in the review request. -- Avoiding jabs on coding style. -- Being clear if you do not have time to do a thorough review. - -## What actions can we take as a team to encourage code reviews? - -- Those in leadership roles should volunteer their time do code reviews, or help find code reviewers, to help embed the culture of code-reviews widely. -- New projects can have a "buddy" who can help to review the code, and means they will be familiar with the protocol and code changes from the start. - diff --git a/mkdocs.yml b/mkdocs.yml index 386ab08bc..04edf8aa7 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -60,7 +60,9 @@ nav: - Publishing your GitHub Repository: publishing-repo.md - Developing a protocol: protocol.md - Using Git effectively: git-workflow.md - - Code reviews: code-reviews.md + - Code reviews: + - Using code reviews: code-reviews.md + - How to request and complete a code review: code-reviews-pull-request.md - Case-control studies: case-control-studies.md - Support: - How to get help: how-to-get-help.md From f2a5550ef1d3c800238095f96d7852082823546e Mon Sep 17 00:00:00 2001 From: Steven Maude Date: Wed, 6 Apr 2022 16:34:51 +0100 Subject: [PATCH 02/11] Rewrite introduction to code reviews slightly * Simplify some of the text. * Explicitly explain what authors and reviewers can gain from code reviews. * Use semantic line breaks. --- docs/code-reviews.md | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/docs/code-reviews.md b/docs/code-reviews.md index 624f1d190..9d011b5a8 100644 --- a/docs/code-reviews.md +++ b/docs/code-reviews.md @@ -1,17 +1,27 @@ ## What is a code review? -A code review is a critical quality assurance practice. It involves systematically -checking a fellow programmer's (or researcher's) code for potential errors or possible -improvements. +A code review is a critical quality assurance practice, commonly used in software development. -It is an everyday activity in the software development world and improves -both quality and speed in programming. Code reviews also offer opportunities for both code author and code reviewer to learn from each other. Code reviews are standard practice within OpenSAFELY studies and the default should be to always get your code reviewed. +A code review typically involves: -## When to ask for code review? +1. checking a fellow programmer's (or researcher's) code for potential errors or possible improvements +2. providing constructive feedback to the author -Code reviews should be the default. A good way of conceptualising when to ask for a review is to think about commits and branches. All your work should be done in branches and you should aim to -merge your branches frequently. A user makes a branch to change a feature or adding some new analysis and make several commits to the new branch. When the time comes to merge this back to main, there is a natural inflexion point to do a code review, and each merge should be -accompanied with a code review. If you are in a hurry, and no-one has time to do a more thorough -code review, bear in mind that even a cursory code review is better than no code review. +Code reviews are standard practice within OpenSAFELY studies. +When making changes to study code, you should always have your proposed changes reviewed. -Merging a new branch should not be your only trigger for a review. If you are stuck or unsure what direction to take, a code review can clarify what path to take. In general, the more frequent the reviews, the better quality the code tends to be. +!!! note + There is [more information on how to request and conduct code reviews](code-reviews-pull-request.md) in this documentation. + +### Benefits of code review for authors and reviewers + +Code reviews are learning and collaboration opportunities for both author and reviewer: + +* *Authors* benefit from having reviewers look at the proposed changes, because the reviewer might: + * spot bugs + * think of wider considerations outside the code change being changed + * think of alternative solutions that could be an improvement +* *Reviewers* benefit from seeing a particular code implementation: + * the reviewer may be less familiar with the project + (but, for critical changes, it is useful to have a reviewer be someone more familiar with the code) + * the reviewer may learn about programming language features, styles or idioms they did not know From 481bbd61fa9ff2d5389c9fc6dc0eeb8aac60f1a6 Mon Sep 17 00:00:00 2001 From: Steven Maude Date: Wed, 6 Apr 2022 16:37:05 +0100 Subject: [PATCH 03/11] Fix some whitespace issues --- docs/code-reviews-pull-request.md | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/docs/code-reviews-pull-request.md b/docs/code-reviews-pull-request.md index 1a13f0fa5..1ec447fab 100644 --- a/docs/code-reviews-pull-request.md +++ b/docs/code-reviews-pull-request.md @@ -1,20 +1,29 @@ ## How to ask for a review -GitHub has a feature built into the workflow that allows easy code review with collaborators, and we would recommend +GitHub has a feature built into the workflow that allows easy code review with collaborators, and we would recommend that you use this tool. GitHub has some [documentation on pull requests and code reviews](https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-request-reviews) which we recommend for background reading. ![An example of GitHub's pull request feature.](./images/code-review-main.png) In this screenshot, the user is making a new pull request. In the top right-hand corner (marked by the -red box), there is a tagging function under Reviewers where you can search or select collaborators to do a code review. +red box), there is a tagging function under Reviewers where you can search or select collaborators to do a code review. Once you have tagged them and made the pull request, the person will receive a notification to their email, and it will also come up in the pull request section of their GitHub main screen. ![An example of a pull request description as shown on GitHub.](./images/pr-desc.png) -It is strongly encouraged to add some information to -your request to aid the reviewer. - +It is strongly encouraged to add some information to +your request to aid the reviewer. + +## When to ask for code review? + +A good way of conceptualising when to ask for a review is to think about commits and branches. All your work should be done in branches and you should aim to +merge your branches frequently. A user makes a branch to change a feature or adding some new analysis and make several commits to the new branch. When the time comes to merge this back to main, there is a natural inflexion point to do a code review, and each merge should be +accompanied with a code review. If you are in a hurry, and no-one has time to do a more thorough +code review, bear in mind that even a cursory code review is better than no code review. + +Merging a new branch should not be your only trigger for a review. If you are stuck or unsure what direction to take, a code review can clarify what path to take. In general, the more frequent the reviews, the better quality the code tends to be. + ## How can I write a good pull request? Some best practice gathered from current users of OpenSAFELY are described below: From bc7b082d6986796b6089c1afc12a00336855d784 Mon Sep 17 00:00:00 2001 From: Steven Maude Date: Wed, 6 Apr 2022 16:39:50 +0100 Subject: [PATCH 04/11] Separate and expand "how to perform a code review" --- docs/code-reviews-pull-request.md | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/docs/code-reviews-pull-request.md b/docs/code-reviews-pull-request.md index 1ec447fab..aba80885c 100644 --- a/docs/code-reviews-pull-request.md +++ b/docs/code-reviews-pull-request.md @@ -43,7 +43,23 @@ will help your reviewer do a better job quicker. ![An example of a good commit message as shown on GitHub.](./images/good-pr-pic.png) ## How can I be a good reviewer? -Some best practices gathered from current users of OpenSAFELY are described below: + +### How to perform a code review + +Each code review might differ in exactly how it is carried out. + +!!! note + Google have some [useful question prompts](https://google.github.io/eng-practices/review/reviewer/looking-for.html) for reviewers to consider. "CL" in that document means "changelist": you can think of "pull request" in place of "changelist". + +#### Resources you have available to you to evaluate the changes + +* The code itself, including any comments. The changes itself should usually be examined by the reviewer. In unusual cases, +* Commit messages in the code. These may give extra context for the reasoning behind changes. +* Comments made in any GitHub issue threads associated or the pull request. +* Running the code to see if the changes. +* Checking there are no newly failing tests because of. These should + +### Some suggestions from current users of OpenSAFELY are described - Code review should be a collaborative conversation and don't be afraid to check assumptions or clarify intentions within the review (you can add messages to the end of the review request). - Actionable feedback with suggestions is helpful. From f9989a7836c54396ecc4d0dcb23ac38100e27ef7 Mon Sep 17 00:00:00 2001 From: Steven Maude Date: Wed, 6 Apr 2022 17:08:56 +0100 Subject: [PATCH 05/11] Fix item list formatting in code reviews page --- docs/code-reviews.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/code-reviews.md b/docs/code-reviews.md index 9d011b5a8..58da9a20b 100644 --- a/docs/code-reviews.md +++ b/docs/code-reviews.md @@ -18,10 +18,11 @@ When making changes to study code, you should always have your proposed changes Code reviews are learning and collaboration opportunities for both author and reviewer: * *Authors* benefit from having reviewers look at the proposed changes, because the reviewer might: - * spot bugs - * think of wider considerations outside the code change being changed - * think of alternative solutions that could be an improvement + * spot bugs + * think of wider considerations outside the code change being changed + * think of alternative solutions that could be an improvement * *Reviewers* benefit from seeing a particular code implementation: - * the reviewer may be less familiar with the project - (but, for critical changes, it is useful to have a reviewer be someone more familiar with the code) - * the reviewer may learn about programming language features, styles or idioms they did not know + * the reviewer may be less familiar with the project + (but, for critical changes, + it is useful to have a reviewer be someone more familiar with the code) + * the reviewer may learn about programming language features, styles or idioms they did not know From 27365d0dd2d48ed36e1046cbe1b935766ba122b3 Mon Sep 17 00:00:00 2001 From: Steven Maude Date: Wed, 6 Apr 2022 17:15:40 +0100 Subject: [PATCH 06/11] Develop the pull request content more * Complete some incomplete thoughts. * Reword a few sentences. * Use more semantic line breaks in edited content. --- docs/code-reviews-pull-request.md | 81 ++++++++++++++++++++----------- 1 file changed, 53 insertions(+), 28 deletions(-) diff --git a/docs/code-reviews-pull-request.md b/docs/code-reviews-pull-request.md index aba80885c..727e3ce64 100644 --- a/docs/code-reviews-pull-request.md +++ b/docs/code-reviews-pull-request.md @@ -6,14 +6,20 @@ that you use this tool. GitHub has some [documentation on pull requests and code ![An example of GitHub's pull request feature.](./images/code-review-main.png) In this screenshot, the user is making a new pull request. In the top right-hand corner (marked by the -red box), there is a tagging function under Reviewers where you can search or select collaborators to do a code review. +red box), there is a tagging function under Reviewers where you can search or select collaborators to request a code review. Once you have tagged them and made the pull request, the person will receive a notification to their -email, and it will also come up in the pull request section of their GitHub main screen. +email, and the notification will also come up in the pull request section of their GitHub main screen. + +!!! note + If you forget to request a review when creating the pull request, you can request after creating the pull request. ![An example of a pull request description as shown on GitHub.](./images/pr-desc.png) -It is strongly encouraged to add some information to -your request to aid the reviewer. +It is strongly encouraged to add some information to the opening message in your pull request to: + +* inform the reviewer what your changes are +* act as a reference for anyone else interested in what changes are proposed +* act as a historical record for anyone looking back at why the changes were made ## When to ask for code review? @@ -28,15 +34,15 @@ Merging a new branch should not be your only trigger for a review. If you are st Some best practice gathered from current users of OpenSAFELY are described below: - - Thinking about the purpose of the review can be helpful as this may -vary from review to review. For example, are you looking for a check of the implementation of the logic or methods -outlined in the protocol (in which case, linking the protocol can aid this), or are you looking for a -review of how well you document your code and if it is clear and understandable? Being transparent -will help your reviewer do a better job quicker. +- Thinking about the purpose of the review can be helpful as this may vary from review to review. + - For example, are you looking for a check of the implementation of the logic or methods outlined in the protocol? + (In which case, linking the protocol can aid this.) + Or are you looking for a review of how well you document your code and if it is clear and understandable? + Being transparent will help your reviewer do a better job quicker. - Good descriptive names for pull requests and commits help the reviewer follow the logic of the code and the changes made (see example of commits below). -- Multiple smaller pull requests are easier than one large. +- Multiple smaller pull requests are easier than one large to review. - Code comments and documentations make the logic easier to follow. -- Naming the order of the files, for example, `01_Data_Extraction.py`, `02_Data_Cleaning.py`, etc. +- Naming data processing files according to the order they are run, for example, `01_Data_Extraction.py`, `02_Data_Cleaning.py`, etc. - Allow sufficient time for the review. ### Examples of good commit messages @@ -46,33 +52,52 @@ will help your reviewer do a better job quicker. ### How to perform a code review -Each code review might differ in exactly how it is carried out. +Each code review might differ in precisely how it is carried out. +You should use your own best judgement when reviewing to consider *what* should be reviewed and *how*. !!! note Google have some [useful question prompts](https://google.github.io/eng-practices/review/reviewer/looking-for.html) for reviewers to consider. "CL" in that document means "changelist": you can think of "pull request" in place of "changelist". #### Resources you have available to you to evaluate the changes -* The code itself, including any comments. The changes itself should usually be examined by the reviewer. In unusual cases, -* Commit messages in the code. These may give extra context for the reasoning behind changes. -* Comments made in any GitHub issue threads associated or the pull request. -* Running the code to see if the changes. -* Checking there are no newly failing tests because of. These should - -### Some suggestions from current users of OpenSAFELY are described - -- Code review should be a collaborative conversation and don't be afraid to check assumptions or clarify intentions within the review (you can add messages to the end of the review request). +* The code itself, including any comments. + The changes should typically usually be examined by the reviewer. + * For more complex changes, you may want to review the changes commit-by-commit: + the Commits tab on the pull request allows you to see view each commit in turn. + * For very extensive changes, it may not be practical to review all the changes; + for example, when automated tools change formatting that are not practical to review line-by-line manually. +* Commit messages the author has written. + These may give extra context for the reasoning behind changes. +* Comments made in any GitHub issue threads associated with the pull request, + or the pull request itself. +* Running the code to see if the changes do what they claim. +* Checking there are no newly failing tests because of the new changes. + * In well-organised projects, there should be automated checks on GitHub that run the tests without you needing to run them manually. + +#### Some suggestions from current users of OpenSAFELY + +- Code review should be a collaborative conversation. + Don't be afraid to check assumptions or clarify intentions within the review. + You can add messages to the end of the review request. - Actionable feedback with suggestions is helpful. -- It can be helpful to highlight the levels of code review comments. For example, -if something is nice-to-have-but-not-important, you can prefix it with `Nit:`. For example, -`Nit: I think this would be cleared if you didn't use an underscore in this variable name` -- Being clear about what exactly you have reviewed or how thorough the review was - for example, being clear if you read through the code but did not run it (and therefore may miss running errors in implementation). Unit tests should catch that the code is runnable, but will not necessarily check it is correct. +- It can be helpful to highlight the levels of code review comments. + If something is nice-to-have-but-not-important, + you can prefix it with `Nit:`. + For example, "Nit: I think this would be cleared if you didn't use an underscore in this variable name". +- Being clear about what exactly you have reviewed or how thorough the review was. + For example, being clear that you read through the code but did not run it + (and therefore may miss running errors in implementation). + Unit tests should catch that the code is runnable, + but will not necessarily check it is correct. - Answer specific questions in the review request. - Avoiding jabs on coding style. - Being clear if you do not have time to do a thorough review. ## What actions can we take as a team to encourage code reviews? -- Those in leadership roles should volunteer their time do code reviews, or help find code reviewers, to help embed the culture of code-reviews widely. -- New projects can have a "buddy" who can help to review the code, and means they will be familiar with the protocol and code changes from the start. - +- Those in leadership roles should volunteer their time to: + - do code reviews + - help find code reviewers + - help embed the culture of code-reviews widely +- New projects can have a "buddy" who can help to review the code, + and means they will be familiar with the protocol and code changes from the start. From 028c672fd7e01eaa2050db4557c291454d083044 Mon Sep 17 00:00:00 2001 From: Steven Maude Date: Wed, 6 Apr 2022 17:22:50 +0100 Subject: [PATCH 07/11] Fix list formatting --- docs/code-reviews-pull-request.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/code-reviews-pull-request.md b/docs/code-reviews-pull-request.md index 727e3ce64..bb874ea35 100644 --- a/docs/code-reviews-pull-request.md +++ b/docs/code-reviews-pull-request.md @@ -96,8 +96,8 @@ You should use your own best judgement when reviewing to consider *what* should ## What actions can we take as a team to encourage code reviews? - Those in leadership roles should volunteer their time to: - - do code reviews - - help find code reviewers - - help embed the culture of code-reviews widely + - do code reviews + - help find code reviewers + - help embed the culture of code-reviews widely - New projects can have a "buddy" who can help to review the code, and means they will be familiar with the protocol and code changes from the start. From dfecc3b879e5e1ae39b643ec83ac676af70186cc Mon Sep 17 00:00:00 2001 From: Steven Maude Date: Wed, 6 Apr 2022 17:24:50 +0100 Subject: [PATCH 08/11] Move small section on encouraging code reviews As it seemed more "about code review" than "how to do a code review". --- docs/code-reviews-pull-request.md | 9 --------- docs/code-reviews.md | 9 +++++++++ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/docs/code-reviews-pull-request.md b/docs/code-reviews-pull-request.md index bb874ea35..6fc1996e4 100644 --- a/docs/code-reviews-pull-request.md +++ b/docs/code-reviews-pull-request.md @@ -92,12 +92,3 @@ You should use your own best judgement when reviewing to consider *what* should - Answer specific questions in the review request. - Avoiding jabs on coding style. - Being clear if you do not have time to do a thorough review. - -## What actions can we take as a team to encourage code reviews? - -- Those in leadership roles should volunteer their time to: - - do code reviews - - help find code reviewers - - help embed the culture of code-reviews widely -- New projects can have a "buddy" who can help to review the code, - and means they will be familiar with the protocol and code changes from the start. diff --git a/docs/code-reviews.md b/docs/code-reviews.md index 58da9a20b..bb551cd4e 100644 --- a/docs/code-reviews.md +++ b/docs/code-reviews.md @@ -26,3 +26,12 @@ Code reviews are learning and collaboration opportunities for both author and re (but, for critical changes, it is useful to have a reviewer be someone more familiar with the code) * the reviewer may learn about programming language features, styles or idioms they did not know + +## What actions can we take as a team to encourage code reviews? + +- Those in leadership roles should volunteer their time to: + - do code reviews + - help find code reviewers + - help embed the culture of code-reviews widely +- New projects can have a "buddy" who can help to review the code, + and means they will be familiar with the protocol and code changes from the start. From a7edfe1736426bc680f2ecd0a2ca5c82530e9362 Mon Sep 17 00:00:00 2001 From: Steven Maude Date: Wed, 6 Apr 2022 17:51:25 +0100 Subject: [PATCH 09/11] Split author and reviewer code review sections --- ...request.md => code-reviews-for-authors.md} | 45 ------------------- docs/code-reviews-for-reviewers.md | 44 ++++++++++++++++++ docs/code-reviews.md | 2 +- mkdocs.yml | 3 +- 4 files changed, 47 insertions(+), 47 deletions(-) rename docs/{code-reviews-pull-request.md => code-reviews-for-authors.md} (57%) create mode 100644 docs/code-reviews-for-reviewers.md diff --git a/docs/code-reviews-pull-request.md b/docs/code-reviews-for-authors.md similarity index 57% rename from docs/code-reviews-pull-request.md rename to docs/code-reviews-for-authors.md index 6fc1996e4..2e236adc2 100644 --- a/docs/code-reviews-pull-request.md +++ b/docs/code-reviews-for-authors.md @@ -47,48 +47,3 @@ Some best practice gathered from current users of OpenSAFELY are described below ### Examples of good commit messages ![An example of a good commit message as shown on GitHub.](./images/good-pr-pic.png) - -## How can I be a good reviewer? - -### How to perform a code review - -Each code review might differ in precisely how it is carried out. -You should use your own best judgement when reviewing to consider *what* should be reviewed and *how*. - -!!! note - Google have some [useful question prompts](https://google.github.io/eng-practices/review/reviewer/looking-for.html) for reviewers to consider. "CL" in that document means "changelist": you can think of "pull request" in place of "changelist". - -#### Resources you have available to you to evaluate the changes - -* The code itself, including any comments. - The changes should typically usually be examined by the reviewer. - * For more complex changes, you may want to review the changes commit-by-commit: - the Commits tab on the pull request allows you to see view each commit in turn. - * For very extensive changes, it may not be practical to review all the changes; - for example, when automated tools change formatting that are not practical to review line-by-line manually. -* Commit messages the author has written. - These may give extra context for the reasoning behind changes. -* Comments made in any GitHub issue threads associated with the pull request, - or the pull request itself. -* Running the code to see if the changes do what they claim. -* Checking there are no newly failing tests because of the new changes. - * In well-organised projects, there should be automated checks on GitHub that run the tests without you needing to run them manually. - -#### Some suggestions from current users of OpenSAFELY - -- Code review should be a collaborative conversation. - Don't be afraid to check assumptions or clarify intentions within the review. - You can add messages to the end of the review request. -- Actionable feedback with suggestions is helpful. -- It can be helpful to highlight the levels of code review comments. - If something is nice-to-have-but-not-important, - you can prefix it with `Nit:`. - For example, "Nit: I think this would be cleared if you didn't use an underscore in this variable name". -- Being clear about what exactly you have reviewed or how thorough the review was. - For example, being clear that you read through the code but did not run it - (and therefore may miss running errors in implementation). - Unit tests should catch that the code is runnable, - but will not necessarily check it is correct. -- Answer specific questions in the review request. -- Avoiding jabs on coding style. -- Being clear if you do not have time to do a thorough review. diff --git a/docs/code-reviews-for-reviewers.md b/docs/code-reviews-for-reviewers.md new file mode 100644 index 000000000..abc1e3861 --- /dev/null +++ b/docs/code-reviews-for-reviewers.md @@ -0,0 +1,44 @@ +## How can I be a good reviewer? + +### How to perform a code review + +Each code review might differ in precisely how it is carried out. +You should use your own best judgement when reviewing to consider *what* should be reviewed and *how*. + +!!! note + Google have some [useful question prompts](https://google.github.io/eng-practices/review/reviewer/looking-for.html) for reviewers to consider. "CL" in that document means "changelist": you can think of "pull request" in place of "changelist". + +#### Resources you have available to you to evaluate the changes + +* The code itself, including any comments. + The changes should typically usually be examined by the reviewer. + * For more complex changes, you may want to review the changes commit-by-commit: + the Commits tab on the pull request allows you to see view each commit in turn. + * For very extensive changes, it may not be practical to review all the changes; + for example, when automated tools change formatting that are not practical to review line-by-line manually. +* Commit messages the author has written. + These may give extra context for the reasoning behind changes. +* Comments made in any GitHub issue threads associated with the pull request, + or the pull request itself. +* Running the code to see if the changes do what they claim. +* Checking there are no newly failing tests because of the new changes. + * In well-organised projects, there should be automated checks on GitHub that run the tests without you needing to run them manually. + +#### Some suggestions from current users of OpenSAFELY + +- Code review should be a collaborative conversation. + Don't be afraid to check assumptions or clarify intentions within the review. + You can add messages to the end of the review request. +- Actionable feedback with suggestions is helpful. +- It can be helpful to highlight the levels of code review comments. + If something is nice-to-have-but-not-important, + you can prefix it with `Nit:`. + For example, "Nit: I think this would be cleared if you didn't use an underscore in this variable name". +- Being clear about what exactly you have reviewed or how thorough the review was. + For example, being clear that you read through the code but did not run it + (and therefore may miss running errors in implementation). + Unit tests should catch that the code is runnable, + but will not necessarily check it is correct. +- Answer specific questions in the review request. +- Avoiding jabs on coding style. +- Being clear if you do not have time to do a thorough review. diff --git a/docs/code-reviews.md b/docs/code-reviews.md index bb551cd4e..2033df005 100644 --- a/docs/code-reviews.md +++ b/docs/code-reviews.md @@ -11,7 +11,7 @@ Code reviews are standard practice within OpenSAFELY studies. When making changes to study code, you should always have your proposed changes reviewed. !!! note - There is [more information on how to request and conduct code reviews](code-reviews-pull-request.md) in this documentation. + There is more information on how to request and conduct code reviews for [authors](code-reviews-for-authors.md) and [reviewers](code-reviews-for-reviewers.md) in this documentation. ### Benefits of code review for authors and reviewers diff --git a/mkdocs.yml b/mkdocs.yml index 04edf8aa7..80b9507d7 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -62,7 +62,8 @@ nav: - Using Git effectively: git-workflow.md - Code reviews: - Using code reviews: code-reviews.md - - How to request and complete a code review: code-reviews-pull-request.md + - How to request a code review: code-reviews-for-authors.md + - How to review code: code-reviews-for-reviewers.md - Case-control studies: case-control-studies.md - Support: - How to get help: how-to-get-help.md From 0ae945256174b6c093108f019903c24755ec114e Mon Sep 17 00:00:00 2001 From: Steven Maude Date: Wed, 6 Apr 2022 17:54:47 +0100 Subject: [PATCH 10/11] Change page title The page no longer really discusses using code reviews, but solely what they are. --- mkdocs.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mkdocs.yml b/mkdocs.yml index 80b9507d7..daa10ad33 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -61,7 +61,7 @@ nav: - Developing a protocol: protocol.md - Using Git effectively: git-workflow.md - Code reviews: - - Using code reviews: code-reviews.md + - About code reviews: code-reviews.md - How to request a code review: code-reviews-for-authors.md - How to review code: code-reviews-for-reviewers.md - Case-control studies: case-control-studies.md From 3039c69c24ba2be40034e7c9eeb49857f0e961da Mon Sep 17 00:00:00 2001 From: Steven Maude Date: Wed, 6 Apr 2022 17:55:52 +0100 Subject: [PATCH 11/11] Reorder sections The "when to ask for code review?" logically should be ahead of the "how?". --- docs/code-reviews-for-authors.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/docs/code-reviews-for-authors.md b/docs/code-reviews-for-authors.md index 2e236adc2..190ab505f 100644 --- a/docs/code-reviews-for-authors.md +++ b/docs/code-reviews-for-authors.md @@ -1,3 +1,12 @@ +## When to ask for code review? + +A good way of conceptualising when to ask for a review is to think about commits and branches. All your work should be done in branches and you should aim to +merge your branches frequently. A user makes a branch to change a feature or adding some new analysis and make several commits to the new branch. When the time comes to merge this back to main, there is a natural inflexion point to do a code review, and each merge should be +accompanied with a code review. If you are in a hurry, and no-one has time to do a more thorough +code review, bear in mind that even a cursory code review is better than no code review. + +Merging a new branch should not be your only trigger for a review. If you are stuck or unsure what direction to take, a code review can clarify what path to take. In general, the more frequent the reviews, the better quality the code tends to be. + ## How to ask for a review GitHub has a feature built into the workflow that allows easy code review with collaborators, and we would recommend @@ -21,15 +30,6 @@ It is strongly encouraged to add some information to the opening message in your * act as a reference for anyone else interested in what changes are proposed * act as a historical record for anyone looking back at why the changes were made -## When to ask for code review? - -A good way of conceptualising when to ask for a review is to think about commits and branches. All your work should be done in branches and you should aim to -merge your branches frequently. A user makes a branch to change a feature or adding some new analysis and make several commits to the new branch. When the time comes to merge this back to main, there is a natural inflexion point to do a code review, and each merge should be -accompanied with a code review. If you are in a hurry, and no-one has time to do a more thorough -code review, bear in mind that even a cursory code review is better than no code review. - -Merging a new branch should not be your only trigger for a review. If you are stuck or unsure what direction to take, a code review can clarify what path to take. In general, the more frequent the reviews, the better quality the code tends to be. - ## How can I write a good pull request? Some best practice gathered from current users of OpenSAFELY are described below: