From 6900c5519adc282c37b765c987f94d089d108a38 Mon Sep 17 00:00:00 2001 From: Maxine Hartnett Date: Mon, 28 Aug 2023 09:55:15 -0600 Subject: [PATCH 1/9] Adding documenation for pull request review standards --- .../checklist-for-pull-requests.rst | 5 +- .../style-guide/review-standards.rst | 51 +++++++++++++++++++ .../development/style-guide/style-guide.rst | 3 +- 3 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 docs/source/development/style-guide/review-standards.rst diff --git a/docs/source/development/style-guide/checklist-for-pull-requests.rst b/docs/source/development/style-guide/checklist-for-pull-requests.rst index 5cd02c01e..c7e365463 100644 --- a/docs/source/development/style-guide/checklist-for-pull-requests.rst +++ b/docs/source/development/style-guide/checklist-for-pull-requests.rst @@ -8,7 +8,9 @@ is only a guide; it should not be treated as a fully comprehensive, foolproof li and parts of it are subjective. If the contributor/reviewer can answer "yes" to all the following questions, then conceivably the proposed changes are -acceptable and the PR can be reviewed and merged. +acceptable and the PR can be reviewed. + +Before merging, make sure you have addressed all the comments in the review per our :ref:`PR standards `. .. _Checklist-for-Contributors: @@ -71,6 +73,7 @@ Pertaining to the pull request: * Does the PR have proper labels? * Is the PR no longer a work in progress? * Do all the automated checks pass? +* Does this post indicate the issue or explain the issue it is solving? .. _pertaining-to-the-code-review: diff --git a/docs/source/development/style-guide/review-standards.rst b/docs/source/development/style-guide/review-standards.rst new file mode 100644 index 000000000..a94c2908a --- /dev/null +++ b/docs/source/development/style-guide/review-standards.rst @@ -0,0 +1,51 @@ +.. _pull-request-review-standards: + +Pull Request and Review Standards +--------------------------------- + +Before any code is merged into our code base, it will need to be put up for pull request (PR) and reviewed. The pull request should be created following +the :ref:`checklist for pull requests `. + +Before opening pull request +=========================== + +Before you create the pull request, you should go through the :ref:`checklist for pull requests ` to ensure +the proposed changes are required and up to our standards. + +If you want to work on the pull request or are not yet finished with the code, please indicate this by marking the pull request as +`wip `_. +Anyone looking at the PR will be able to quickly see it is not yet ready for review. + +Finally, if you are addressing an existing issue, make sure that issue is linked in your PR. If there is not an existing issue, then you should either create an issue or address WHY you are opening the PR specifically. + +During review +============= + +As a reviewer, please follow these rules of thumb: + +#. Comments should be clear in addressing why you want to see the change +#. Comments should be polite, but straightfoward and candid +#. If you leave a review, continue to follow up on replies to your questions or comments and review the changes you requested +#. It is polite, but not required, to provide examples for suggestions (particularly for things like name changes) + +Before merging +============== + +Before merging, a pull request needs one approving review. While the review is open, anyone can make comments or request changes on the PR. + +Although only one approval is required, you must follow these rules: + +#. If there is someone with a particular expertise or vested interest in your changes, **do not merge or close the pull request until they get a chance to review.** +#. Do not merge until you have addressed all the comments on your review, even if you have an approval from someone. +#. If someone asked for changes beyond a nitpick, do not merge until you have an approval or thumbs up from them. This does not mean you need to change your code if you don't agree with them, but you should explain why you will not be making the changes and make sure they are ok with merging anyway. +#. You should go through the :ref:`pull request checklist ` +#. You should ensure ALL checks on the PR pass (tests are passing) +#. If you have a lot of commits, clean up the commits by running a `rebase `_ and combining commits. + +After merging +============= + +Once you have merged your code: + +#. Close the issue (if it exists) +#. Remove your branch if it is no longer needed \ No newline at end of file diff --git a/docs/source/development/style-guide/style-guide.rst b/docs/source/development/style-guide/style-guide.rst index d39bbcfeb..fe95402a9 100644 --- a/docs/source/development/style-guide/style-guide.rst +++ b/docs/source/development/style-guide/style-guide.rst @@ -41,4 +41,5 @@ to these conventions. naming-conventions tools-and-library-recommendations versioning - checklist-for-pull-requests \ No newline at end of file + checklist-for-pull-requests + review-standards \ No newline at end of file From 4add3d6f9781ac9140e4bac53963d88f6ee17e4a Mon Sep 17 00:00:00 2001 From: Maxine Hartnett Date: Mon, 28 Aug 2023 10:36:34 -0600 Subject: [PATCH 2/9] addressing PR comments --- .../development/style-guide/review-standards.rst | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/docs/source/development/style-guide/review-standards.rst b/docs/source/development/style-guide/review-standards.rst index a94c2908a..1ee382a51 100644 --- a/docs/source/development/style-guide/review-standards.rst +++ b/docs/source/development/style-guide/review-standards.rst @@ -12,11 +12,12 @@ Before opening pull request Before you create the pull request, you should go through the :ref:`checklist for pull requests ` to ensure the proposed changes are required and up to our standards. -If you want to work on the pull request or are not yet finished with the code, please indicate this by marking the pull request as -`wip `_. +If you want to work on the pull request or are not yet finished with the code, please indicate this by marking the pull request as a +`draft or WIP `_ PR. Anyone looking at the PR will be able to quickly see it is not yet ready for review. Finally, if you are addressing an existing issue, make sure that issue is linked in your PR. If there is not an existing issue, then you should either create an issue or address WHY you are opening the PR specifically. +If your PR addresses a `Level-5 requirement `_, there **must** be a corresponding issue linked. During review ============= @@ -28,6 +29,11 @@ As a reviewer, please follow these rules of thumb: #. If you leave a review, continue to follow up on replies to your questions or comments and review the changes you requested #. It is polite, but not required, to provide examples for suggestions (particularly for things like name changes) +As an author: + +#. If you would like to request a specific review from someone, make sure they are marked as a reviewer or called out in a comment on the review (by typing ``@``) +#. + Before merging ============== @@ -37,11 +43,13 @@ Although only one approval is required, you must follow these rules: #. If there is someone with a particular expertise or vested interest in your changes, **do not merge or close the pull request until they get a chance to review.** #. Do not merge until you have addressed all the comments on your review, even if you have an approval from someone. +#. Make sure you have left the review open for a sufficent amount of time to allow people to review - usually 3-5 days is enough. #. If someone asked for changes beyond a nitpick, do not merge until you have an approval or thumbs up from them. This does not mean you need to change your code if you don't agree with them, but you should explain why you will not be making the changes and make sure they are ok with merging anyway. #. You should go through the :ref:`pull request checklist ` #. You should ensure ALL checks on the PR pass (tests are passing) #. If you have a lot of commits, clean up the commits by running a `rebase `_ and combining commits. + After merging ============= From dccad9ee4fb94afde2f8229bf65d0a401c5d442a Mon Sep 17 00:00:00 2001 From: Maxine Hartnett Date: Mon, 28 Aug 2023 12:21:53 -0600 Subject: [PATCH 3/9] Adding notes from meeting discussion --- docs/source/development/style-guide/review-standards.rst | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/source/development/style-guide/review-standards.rst b/docs/source/development/style-guide/review-standards.rst index 1ee382a51..fc04de543 100644 --- a/docs/source/development/style-guide/review-standards.rst +++ b/docs/source/development/style-guide/review-standards.rst @@ -12,6 +12,9 @@ Before opening pull request Before you create the pull request, you should go through the :ref:`checklist for pull requests ` to ensure the proposed changes are required and up to our standards. +The PR should be as small as possible. However, the code included in the PR should be complete. It should complete an entire feature, but this doesn't necessarily mean it completes an entire issue. +However, all code merged into the ``dev`` branch should be up to our standards and work sufficiently. + If you want to work on the pull request or are not yet finished with the code, please indicate this by marking the pull request as a `draft or WIP `_ PR. Anyone looking at the PR will be able to quickly see it is not yet ready for review. @@ -28,11 +31,13 @@ As a reviewer, please follow these rules of thumb: #. Comments should be polite, but straightfoward and candid #. If you leave a review, continue to follow up on replies to your questions or comments and review the changes you requested #. It is polite, but not required, to provide examples for suggestions (particularly for things like name changes) +#. If you require a change to be addressed, add a "request changes" comment. If you make one of these comments, it means you are blocking the code review from merging until that change is addressed. +#. If you make a "request changes" comment, you must create a follow up review where you change that to an approving review. Please do this in a timely matter so you do not block the PR for longer than necessary As an author: #. If you would like to request a specific review from someone, make sure they are marked as a reviewer or called out in a comment on the review (by typing ``@``) -#. +#. You can request a review from the entire team or from a specific instrument team using the team ``IMAP-Science-Operations-Center/imap-sdc`` Before merging ============== @@ -48,6 +53,7 @@ Although only one approval is required, you must follow these rules: #. You should go through the :ref:`pull request checklist ` #. You should ensure ALL checks on the PR pass (tests are passing) #. If you have a lot of commits, clean up the commits by running a `rebase `_ and combining commits. +#. At least one approval should come from the SDC team. Comments from external people should be treated the same way SDC comments are. After merging From 9b3a8d3cd2f4239bb828500629b89ac8d8ed9d18 Mon Sep 17 00:00:00 2001 From: Maxine Hartnett Date: Mon, 28 Aug 2023 16:06:59 -0600 Subject: [PATCH 4/9] removing unneeded section, adding link --- .../style-guide/review-standards.rst | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/docs/source/development/style-guide/review-standards.rst b/docs/source/development/style-guide/review-standards.rst index fc04de543..9d6fb22d0 100644 --- a/docs/source/development/style-guide/review-standards.rst +++ b/docs/source/development/style-guide/review-standards.rst @@ -19,7 +19,7 @@ If you want to work on the pull request or are not yet finished with the code, p `draft or WIP `_ PR. Anyone looking at the PR will be able to quickly see it is not yet ready for review. -Finally, if you are addressing an existing issue, make sure that issue is linked in your PR. If there is not an existing issue, then you should either create an issue or address WHY you are opening the PR specifically. +Finally, if you are addressing an existing issue, make sure that issue is `linked `_ in your PR. If there is not an existing issue, then you should either create an issue or address WHY you are opening the PR specifically. If your PR addresses a `Level-5 requirement `_, there **must** be a corresponding issue linked. During review @@ -30,15 +30,21 @@ As a reviewer, please follow these rules of thumb: #. Comments should be clear in addressing why you want to see the change #. Comments should be polite, but straightfoward and candid #. If you leave a review, continue to follow up on replies to your questions or comments and review the changes you requested -#. It is polite, but not required, to provide examples for suggestions (particularly for things like name changes) +#. It is nice, but not required, to provide examples for suggestions (particularly for things like name changes) #. If you require a change to be addressed, add a "request changes" comment. If you make one of these comments, it means you are blocking the code review from merging until that change is addressed. -#. If you make a "request changes" comment, you must create a follow up review where you change that to an approving review. Please do this in a timely matter so you do not block the PR for longer than necessary +#. If you make a "request changes" comment, you must create a follow up review where you change that to an approving review (or make another "request changes" review). Please do this in a timely matter so you do not block the PR for longer than necessary As an author: #. If you would like to request a specific review from someone, make sure they are marked as a reviewer or called out in a comment on the review (by typing ``@``) #. You can request a review from the entire team or from a specific instrument team using the team ``IMAP-Science-Operations-Center/imap-sdc`` +As a team: + +#. All parties need to be respectful during code reviews +#. Don't take comments personally - treat everyone as a fellow team member working to produce excellent code, not as adversaries to defeat before you can merge +#. Be honest - if you disagree with someones comment, start a discussion on why that is the case + Before merging ============== @@ -54,12 +60,3 @@ Although only one approval is required, you must follow these rules: #. You should ensure ALL checks on the PR pass (tests are passing) #. If you have a lot of commits, clean up the commits by running a `rebase `_ and combining commits. #. At least one approval should come from the SDC team. Comments from external people should be treated the same way SDC comments are. - - -After merging -============= - -Once you have merged your code: - -#. Close the issue (if it exists) -#. Remove your branch if it is no longer needed \ No newline at end of file From d05fa5bd7b8317eeb772cbcf7eee0cfde8124f48 Mon Sep 17 00:00:00 2001 From: Maxine Hartnett <117409426+maxinelasp@users.noreply.github.com> Date: Mon, 28 Aug 2023 14:06:21 -0600 Subject: [PATCH 5/9] Spelling fixes and updates from PR Co-authored-by: Matthew Bourque , Greg Lucas --- docs/source/development/style-guide/review-standards.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/source/development/style-guide/review-standards.rst b/docs/source/development/style-guide/review-standards.rst index 9d6fb22d0..1129848fa 100644 --- a/docs/source/development/style-guide/review-standards.rst +++ b/docs/source/development/style-guide/review-standards.rst @@ -3,14 +3,14 @@ Pull Request and Review Standards --------------------------------- -Before any code is merged into our code base, it will need to be put up for pull request (PR) and reviewed. The pull request should be created following +Before any code is merged into the code base, it will need to be put up for pull request (PR) and reviewed. The pull request should be created following the :ref:`checklist for pull requests `. -Before opening pull request +Before opening a pull request =========================== Before you create the pull request, you should go through the :ref:`checklist for pull requests ` to ensure -the proposed changes are required and up to our standards. +the proposed changes meet the repository standards. The PR should be as small as possible. However, the code included in the PR should be complete. It should complete an entire feature, but this doesn't necessarily mean it completes an entire issue. However, all code merged into the ``dev`` branch should be up to our standards and work sufficiently. @@ -28,7 +28,7 @@ During review As a reviewer, please follow these rules of thumb: #. Comments should be clear in addressing why you want to see the change -#. Comments should be polite, but straightfoward and candid +#. Comments should be polite, but straightforward and candid #. If you leave a review, continue to follow up on replies to your questions or comments and review the changes you requested #. It is nice, but not required, to provide examples for suggestions (particularly for things like name changes) #. If you require a change to be addressed, add a "request changes" comment. If you make one of these comments, it means you are blocking the code review from merging until that change is addressed. From b82fd6f2290ff29eb1bdb5624a6436edc8b113d6 Mon Sep 17 00:00:00 2001 From: Maxine Hartnett <117409426+maxinelasp@users.noreply.github.com> Date: Tue, 29 Aug 2023 08:33:34 -0600 Subject: [PATCH 6/9] Update docs/source/development/style-guide/review-standards.rst Co-authored-by: Greg Lucas --- docs/source/development/style-guide/review-standards.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/development/style-guide/review-standards.rst b/docs/source/development/style-guide/review-standards.rst index 1129848fa..288aa6fda 100644 --- a/docs/source/development/style-guide/review-standards.rst +++ b/docs/source/development/style-guide/review-standards.rst @@ -13,7 +13,7 @@ Before you create the pull request, you should go through the :ref:`checklist fo the proposed changes meet the repository standards. The PR should be as small as possible. However, the code included in the PR should be complete. It should complete an entire feature, but this doesn't necessarily mean it completes an entire issue. -However, all code merged into the ``dev`` branch should be up to our standards and work sufficiently. +However, all code merged into the ``dev`` branch should meet the repository standards and work as expected. If you want to work on the pull request or are not yet finished with the code, please indicate this by marking the pull request as a `draft or WIP `_ PR. From 80bfe3ed86e2dbe8491d1ae11fc8be4bb3a2389f Mon Sep 17 00:00:00 2001 From: Maxine Hartnett <117409426+maxinelasp@users.noreply.github.com> Date: Tue, 29 Aug 2023 14:03:07 -0600 Subject: [PATCH 7/9] Update docs/source/development/style-guide/review-standards.rst Co-authored-by: Greg Lucas --- docs/source/development/style-guide/review-standards.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/development/style-guide/review-standards.rst b/docs/source/development/style-guide/review-standards.rst index 288aa6fda..f655932ef 100644 --- a/docs/source/development/style-guide/review-standards.rst +++ b/docs/source/development/style-guide/review-standards.rst @@ -7,7 +7,7 @@ Before any code is merged into the code base, it will need to be put up for pull the :ref:`checklist for pull requests `. Before opening a pull request -=========================== +============================= Before you create the pull request, you should go through the :ref:`checklist for pull requests ` to ensure the proposed changes meet the repository standards. From d3c3ec3a1a4d676e6ee046a9809edf4ffbadc6ee Mon Sep 17 00:00:00 2001 From: Maxine Hartnett <117409426+maxinelasp@users.noreply.github.com> Date: Tue, 29 Aug 2023 14:03:14 -0600 Subject: [PATCH 8/9] Update docs/source/development/style-guide/checklist-for-pull-requests.rst Co-authored-by: Greg Lucas --- .../development/style-guide/checklist-for-pull-requests.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/development/style-guide/checklist-for-pull-requests.rst b/docs/source/development/style-guide/checklist-for-pull-requests.rst index c7e365463..c9f165fe7 100644 --- a/docs/source/development/style-guide/checklist-for-pull-requests.rst +++ b/docs/source/development/style-guide/checklist-for-pull-requests.rst @@ -10,7 +10,7 @@ and parts of it are subjective. If the contributor/reviewer can answer "yes" to all the following questions, then conceivably the proposed changes are acceptable and the PR can be reviewed. -Before merging, make sure you have addressed all the comments in the review per our :ref:`PR standards `. +Before merging, make sure you have addressed all the comments in the review adhering to the :ref:`PR standards `. .. _Checklist-for-Contributors: From 4dd847831bc9c0a91922825ea7c305ab252376db Mon Sep 17 00:00:00 2001 From: Maxine Hartnett Date: Wed, 30 Aug 2023 08:42:46 -0600 Subject: [PATCH 9/9] Clarifying draft PR paragraph --- docs/source/development/style-guide/review-standards.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/source/development/style-guide/review-standards.rst b/docs/source/development/style-guide/review-standards.rst index f655932ef..3d9b28faa 100644 --- a/docs/source/development/style-guide/review-standards.rst +++ b/docs/source/development/style-guide/review-standards.rst @@ -17,7 +17,8 @@ However, all code merged into the ``dev`` branch should meet the repository stan If you want to work on the pull request or are not yet finished with the code, please indicate this by marking the pull request as a `draft or WIP `_ PR. -Anyone looking at the PR will be able to quickly see it is not yet ready for review. +Anyone looking at the PR will be able to quickly see it is not final. Marking your PR as draft means you are asking someone for an initial review, or if you want to get comments on your +initial design. If you put up a draft PR, indicate whether or not you are looking for initial reviews in the summary. Finally, if you are addressing an existing issue, make sure that issue is `linked `_ in your PR. If there is not an existing issue, then you should either create an issue or address WHY you are opening the PR specifically. If your PR addresses a `Level-5 requirement `_, there **must** be a corresponding issue linked.