From 7e0a5f332596dea1a18631edf9e58a2b877e8f2f Mon Sep 17 00:00:00 2001 From: Jochen Martin Eppler Date: Fri, 18 Mar 2022 15:41:38 +0100 Subject: [PATCH 1/5] Update review guidelines --- .../contribute/code_review_guidelines.rst | 55 ++++++++++++------- 1 file changed, 34 insertions(+), 21 deletions(-) diff --git a/doc/userdoc/contribute/code_review_guidelines.rst b/doc/userdoc/contribute/code_review_guidelines.rst index df2ea6a142..fae8ae68af 100644 --- a/doc/userdoc/contribute/code_review_guidelines.rst +++ b/doc/userdoc/contribute/code_review_guidelines.rst @@ -13,24 +13,37 @@ meant to prevent progress, but to keep up the code quality as the number of developers is growing. All of this is not set in stone and can be discussed on the :ref:`NEST mailing lists `. -* In general, the rule is that each pull request needs an OK from the CI - platform and at least two reviewers to be merged. -* For changes labeled “not code” or “minor” (e.g., changes in documentation, - fixes for typos, etc.), the release manager can waive the need for code - review and just accept the OK from the CI system in order to merge the request. -* Each pull request needs to be documented by an issue in the `issue - tracker `_ explaining the reason - for the changes and the solution. The issue is also the place for discussions - about the code. -* New features like SLI or PyNEST functions, neuron or synapse models need to - be accompanied by one or more tests written either in SLI or Python. New - features for the NEST kernel need a test written in SLI. -* Each change to the code has to be reflected also in the corresponding - examples and documentation. -* All source code has to be adhering to the Coding Guidelines for - :ref:`C++ ` in order to - pass the :ref:`continuous integration system `. -* All commits should be coherent and contain only changes that belong together. -* Please check the **typeset documentation** as part of the review process. To - learn how to test the documentation locally offline, please check out our - :ref:`User documentation workflow `. +* In general, the rule is that each pull request needs an OK from the + CI platform and at least two reviewers to be merged. +* The two reviews have to cover the technical side (i.e., if the code + does the right thing and is architecturally sane) and the content + side (i.e., if the code is scientifically correct and fixes an + actual issue) +* For changes labeled “not code” or “minor” (e.g., changes in + documentation, fixes for typos, etc.), the need for a second code + review can be waived and a single rewiew plus the OK from the CI + system is sufficient in order to merge the request. +* New features like SLI or PyNEST functions, neuron or synapse models + need to be accompanied by one or more tests written either in SLI or + Python. +* Each change to the code has to be reflected also in the + corresponding examples and documentation. +* A pull request should be coherent and contain only changes that + belong together. +* Please check the **typeset documentation** as part of the review + process. To learn how to test the documentation locally offline, + please check out our :ref:`User documentation workflow + `. + + +At the latest before merging, reviewers have to make sure that: + +1. pull request titles have to match the actual content of the PR and + be adequate for the release notes +1. pull request titles are complete sentences that start with an + upper-case verb and end without punctuation +1. pull requests are assigned to projects and properly and completely + labelled +1. all discussions are settled and all conversations are marked as + resolved +1. there are no blocking issues mentioned in the comments From c6b769592c15e271669aadfed18fc5b3bec46219 Mon Sep 17 00:00:00 2001 From: Jochen Martin Eppler Date: Fri, 18 Mar 2022 16:07:22 +0100 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: jessica-mitchell --- .../contribute/code_review_guidelines.rst | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/doc/userdoc/contribute/code_review_guidelines.rst b/doc/userdoc/contribute/code_review_guidelines.rst index fae8ae68af..c34f03e5e8 100644 --- a/doc/userdoc/contribute/code_review_guidelines.rst +++ b/doc/userdoc/contribute/code_review_guidelines.rst @@ -13,21 +13,21 @@ meant to prevent progress, but to keep up the code quality as the number of developers is growing. All of this is not set in stone and can be discussed on the :ref:`NEST mailing lists `. -* In general, the rule is that each pull request needs an OK from the +* In most cases, each pull request needs an OK from the CI platform and at least two reviewers to be merged. * The two reviews have to cover the technical side (i.e., if the code does the right thing and is architecturally sane) and the content side (i.e., if the code is scientifically correct and fixes an - actual issue) + actual issue). * For changes labeled “not code” or “minor” (e.g., changes in documentation, fixes for typos, etc.), the need for a second code - review can be waived and a single rewiew plus the OK from the CI - system is sufficient in order to merge the request. + review can be waived, and a single review plus the OK from the CI + system is sufficient to merge the request. * New features like SLI or PyNEST functions, neuron or synapse models - need to be accompanied by one or more tests written either in SLI or + need to be accompanied by one or more tests written in either SLI or Python. -* Each change to the code has to be reflected also in the - corresponding examples and documentation. +* Each change to the code has to be reflected in the + corresponding examples and documentation, as well. * A pull request should be coherent and contain only changes that belong together. * Please check the **typeset documentation** as part of the review @@ -36,14 +36,14 @@ can be discussed on the :ref:`NEST mailing lists `. `. -At the latest before merging, reviewers have to make sure that: +Before merging, reviewers have to make sure that: -1. pull request titles have to match the actual content of the PR and +1. pull request titles match the actual content of the PR and be adequate for the release notes 1. pull request titles are complete sentences that start with an - upper-case verb and end without punctuation + upper-case, present tense verb and end without punctuation 1. pull requests are assigned to projects and properly and completely - labelled + labeled 1. all discussions are settled and all conversations are marked as resolved 1. there are no blocking issues mentioned in the comments From 18756016714b5f74eae4d8b909058a2232fe088d Mon Sep 17 00:00:00 2001 From: Jochen Martin Eppler Date: Wed, 23 Mar 2022 11:21:01 +0100 Subject: [PATCH 3/5] Update doc/userdoc/contribute/code_review_guidelines.rst Co-authored-by: jessica-mitchell --- doc/userdoc/contribute/code_review_guidelines.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/userdoc/contribute/code_review_guidelines.rst b/doc/userdoc/contribute/code_review_guidelines.rst index c34f03e5e8..556a542016 100644 --- a/doc/userdoc/contribute/code_review_guidelines.rst +++ b/doc/userdoc/contribute/code_review_guidelines.rst @@ -30,7 +30,7 @@ can be discussed on the :ref:`NEST mailing lists `. corresponding examples and documentation, as well. * A pull request should be coherent and contain only changes that belong together. -* Please check the **typeset documentation** as part of the review +* Please also check that the typesetting of the documentation looks correct. process. To learn how to test the documentation locally offline, please check out our :ref:`User documentation workflow `. From f3b8fce86ce1812c716e505bf309ef867e22358e Mon Sep 17 00:00:00 2001 From: Jochen Martin Eppler Date: Wed, 23 Mar 2022 11:22:13 +0100 Subject: [PATCH 4/5] Update doc/userdoc/contribute/code_review_guidelines.rst --- doc/userdoc/contribute/code_review_guidelines.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/userdoc/contribute/code_review_guidelines.rst b/doc/userdoc/contribute/code_review_guidelines.rst index 556a542016..1518bb6b5d 100644 --- a/doc/userdoc/contribute/code_review_guidelines.rst +++ b/doc/userdoc/contribute/code_review_guidelines.rst @@ -30,8 +30,8 @@ can be discussed on the :ref:`NEST mailing lists `. corresponding examples and documentation, as well. * A pull request should be coherent and contain only changes that belong together. -* Please also check that the typesetting of the documentation looks correct. - process. To learn how to test the documentation locally offline, +* Please also check that the typesetting of the documentation looks + correct. To learn how to test the documentation locally offline, please check out our :ref:`User documentation workflow `. From c60ccde1589b09eb0a519404e3903fa3bb35daca Mon Sep 17 00:00:00 2001 From: Jochen Martin Eppler Date: Wed, 27 Apr 2022 15:18:44 +0200 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Hans Ekkehard Plesser --- doc/userdoc/contribute/code_review_guidelines.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/userdoc/contribute/code_review_guidelines.rst b/doc/userdoc/contribute/code_review_guidelines.rst index 1518bb6b5d..f611f5930c 100644 --- a/doc/userdoc/contribute/code_review_guidelines.rst +++ b/doc/userdoc/contribute/code_review_guidelines.rst @@ -16,7 +16,7 @@ can be discussed on the :ref:`NEST mailing lists `. * In most cases, each pull request needs an OK from the CI platform and at least two reviewers to be merged. * The two reviews have to cover the technical side (i.e., if the code - does the right thing and is architecturally sane) and the content + does the right thing and is architecturally sound) and the content side (i.e., if the code is scientifically correct and fixes an actual issue). * For changes labeled “not code” or “minor” (e.g., changes in @@ -27,7 +27,7 @@ can be discussed on the :ref:`NEST mailing lists `. need to be accompanied by one or more tests written in either SLI or Python. * Each change to the code has to be reflected in the - corresponding examples and documentation, as well. + corresponding examples and documentation. * A pull request should be coherent and contain only changes that belong together. * Please also check that the typesetting of the documentation looks @@ -41,7 +41,7 @@ Before merging, reviewers have to make sure that: 1. pull request titles match the actual content of the PR and be adequate for the release notes 1. pull request titles are complete sentences that start with an - upper-case, present tense verb and end without punctuation + upper-case, present-tense verb and end without punctuation 1. pull requests are assigned to projects and properly and completely labeled 1. all discussions are settled and all conversations are marked as