From 3816ad8d4e4e3c3017e492b7fb82a741538f82fb Mon Sep 17 00:00:00 2001 From: Lorena Pantano Date: Wed, 9 Oct 2024 11:05:28 -0400 Subject: [PATCH 01/10] add draft to guidelines from slack discussion --- .../contributing_to_pipelines.md | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md index 19c205349d..b6b303c17e 100644 --- a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md +++ b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md @@ -42,7 +42,7 @@ If you'd like to write some code for an nf-core pipeline, the standard workflow 2. [Fork](https://help.github.com/en/github/getting-started-with-github/fork-a-repo) the pipeline repository to your GitHub account 3. Make the necessary changes / additions on `dev` branch by using [git checkout](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/committing-changes-to-a-pull-request-branch-created-from-a-fork) within your forked repository following [pipeline conventions](#pipeline-contribution-conventions) 4. Use `nf-core pipelines schema build` and add any new parameters to the pipeline JSON schema (requires [nf-core tools](https://github.com/nf-core/tools) >= 1.10). -5. Submit a Pull Request against the `dev` branch and wait for the code to be reviewed and merged +5. Submit a Pull Request against the `dev` branch and wait for the code to be reviewed and merged. See below guidelines for this. If you're new to working with git, you can view the [GitHub pull requests documentation](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests) or other [excellent `git` resources](https://try.github.io/) to get started. @@ -80,6 +80,20 @@ Only in the unlikely and regretful event of a release happening with a bug. - Fix the bug, and bump version (X.Y.Z+1). - A PR should be made on `master` from patch to directly resolve this particular bug. +## PR review guidelines + +- Authors/Reviewers can request help for review at nf-core Slack `#request-review` channel. +- Anyone can merge after one positive review and with no obvious open questions and, merge ASAP so as not to leave approved PRs hanging. +- Exception in merges to master for releases (two reviews). +- Exception in pseudo PR review for first release (needs review from core or maintainer team). +- If it's a major change then it's ok to request a second review - either the author or reviewer can do this. +- PRs that have approving reviews but open questions can be merged by the author after addressing the questions to a common-sense level of satisfaction. +- If the reviewer feels uneasy about this then they shouldn't leave an approval, but rather a request-changes. +- We prefer to use merge commits to merge. This is to avoid merge conflicts when multiple people are pulling with overlapping feature branches. +- We prefer verbose commit histories but easy merges. +- Feature branches should be immediately deleted after merge. Ideally by selecting the new feature on GitHub repo settings +- Note: squash merging commits in a PR and merging after is fine, that is up to the individuals. + ## Getting help For further information/help, please consult the usage documentation for the particular pipeline and don't hesitate to get in touch on the nf-core Slack `#` channel. If you are not already a member you can ([join our Slack here](https://nf-co.re/join/slack)). From 5ab97e43bcbcf387ff5f6edc4b3bb803a1bc1f87 Mon Sep 17 00:00:00 2001 From: Lorena Pantano Date: Wed, 9 Oct 2024 11:24:14 -0400 Subject: [PATCH 02/10] add new section to toc --- .../contributing_to_nf-core/contributing_to_pipelines.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md index b6b303c17e..a32f36163a 100644 --- a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md +++ b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md @@ -19,8 +19,9 @@ The main steps involved in adding a new nf-core pipeline covered below are: 2. [Contribution overview](#contribution-overview) 3. [Testing](#testing) 4. [Patching bugs](#patching-bugs) -5. [Getting help](#getting-help) -6. [Pipeline contribution conventions](#pipeline-contribution-conventions) +5. [PR review guidelines](##pr-review-guidelines) +6. [Getting help](#getting-help) +7. [Pipeline contribution conventions](#pipeline-contribution-conventions) ## Join the community From abc438fbaab8117da27e9d92a53d5a8c010f187d Mon Sep 17 00:00:00 2001 From: Lorena Pantano Date: Wed, 9 Oct 2024 16:43:03 -0400 Subject: [PATCH 03/10] Update sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md Co-authored-by: James A. Fellows Yates --- .../contributing_to_nf-core/contributing_to_pipelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md index a32f36163a..a9fb1034e6 100644 --- a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md +++ b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md @@ -85,7 +85,7 @@ Only in the unlikely and regretful event of a release happening with a bug. - Authors/Reviewers can request help for review at nf-core Slack `#request-review` channel. - Anyone can merge after one positive review and with no obvious open questions and, merge ASAP so as not to leave approved PRs hanging. -- Exception in merges to master for releases (two reviews). +- Exception: merges to master for releases requires **two** reviews. - Exception in pseudo PR review for first release (needs review from core or maintainer team). - If it's a major change then it's ok to request a second review - either the author or reviewer can do this. - PRs that have approving reviews but open questions can be merged by the author after addressing the questions to a common-sense level of satisfaction. From 388c3cbe679c4525e042eaabb025668230db471a Mon Sep 17 00:00:00 2001 From: Lorena Pantano Date: Wed, 9 Oct 2024 16:43:39 -0400 Subject: [PATCH 04/10] fix grammar in exception Co-authored-by: James A. Fellows Yates --- .../contributing_to_nf-core/contributing_to_pipelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md index a9fb1034e6..6d779438fd 100644 --- a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md +++ b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md @@ -86,7 +86,7 @@ Only in the unlikely and regretful event of a release happening with a bug. - Authors/Reviewers can request help for review at nf-core Slack `#request-review` channel. - Anyone can merge after one positive review and with no obvious open questions and, merge ASAP so as not to leave approved PRs hanging. - Exception: merges to master for releases requires **two** reviews. -- Exception in pseudo PR review for first release (needs review from core or maintainer team). +- Exception: pseudo PR reviews for a first release needs **review from core or maintainer team**. - If it's a major change then it's ok to request a second review - either the author or reviewer can do this. - PRs that have approving reviews but open questions can be merged by the author after addressing the questions to a common-sense level of satisfaction. - If the reviewer feels uneasy about this then they shouldn't leave an approval, but rather a request-changes. From 267d98ea8feacef0feb92e6aa14238f0f1efc86a Mon Sep 17 00:00:00 2001 From: Lorena Pantano Date: Wed, 9 Oct 2024 16:44:07 -0400 Subject: [PATCH 05/10] fix grammar in major change case Co-authored-by: James A. Fellows Yates --- .../contributing_to_nf-core/contributing_to_pipelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md index 6d779438fd..ede98620d2 100644 --- a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md +++ b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md @@ -87,7 +87,7 @@ Only in the unlikely and regretful event of a release happening with a bug. - Anyone can merge after one positive review and with no obvious open questions and, merge ASAP so as not to leave approved PRs hanging. - Exception: merges to master for releases requires **two** reviews. - Exception: pseudo PR reviews for a first release needs **review from core or maintainer team**. -- If it's a major change then it's ok to request a second review - either the author or reviewer can do this. +- If it's there is a major change in any type of PR (from modules to pipelines) then it's ok to request a second review - either the author or reviewer can do this. - PRs that have approving reviews but open questions can be merged by the author after addressing the questions to a common-sense level of satisfaction. - If the reviewer feels uneasy about this then they shouldn't leave an approval, but rather a request-changes. - We prefer to use merge commits to merge. This is to avoid merge conflicts when multiple people are pulling with overlapping feature branches. From 554a16e437367ace127f9c050ce7828c94a8f107 Mon Sep 17 00:00:00 2001 From: Lorena Pantano Date: Wed, 9 Oct 2024 16:44:22 -0400 Subject: [PATCH 06/10] fix grammar in open questions case Co-authored-by: James A. Fellows Yates --- .../contributing_to_nf-core/contributing_to_pipelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md index ede98620d2..cb6d8b7289 100644 --- a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md +++ b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md @@ -88,7 +88,7 @@ Only in the unlikely and regretful event of a release happening with a bug. - Exception: merges to master for releases requires **two** reviews. - Exception: pseudo PR reviews for a first release needs **review from core or maintainer team**. - If it's there is a major change in any type of PR (from modules to pipelines) then it's ok to request a second review - either the author or reviewer can do this. -- PRs that have approving reviews but open questions can be merged by the author after addressing the questions to a common-sense level of satisfaction. +- PRs that have approving reviews but remaining open questions can be merged by the author after addressing the questions to a common-sense level of satisfaction. - If the reviewer feels uneasy about this then they shouldn't leave an approval, but rather a request-changes. - We prefer to use merge commits to merge. This is to avoid merge conflicts when multiple people are pulling with overlapping feature branches. - We prefer verbose commit histories but easy merges. From 0ffc349179acd7c74f7c8df110e511b1f28019e5 Mon Sep 17 00:00:00 2001 From: Lorena Pantano Date: Wed, 9 Oct 2024 16:44:58 -0400 Subject: [PATCH 07/10] fix structure --- .../contributing_to_pipelines.md | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md index a32f36163a..90fe00bc4b 100644 --- a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md +++ b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md @@ -84,16 +84,17 @@ Only in the unlikely and regretful event of a release happening with a bug. ## PR review guidelines - Authors/Reviewers can request help for review at nf-core Slack `#request-review` channel. -- Anyone can merge after one positive review and with no obvious open questions and, merge ASAP so as not to leave approved PRs hanging. -- Exception in merges to master for releases (two reviews). -- Exception in pseudo PR review for first release (needs review from core or maintainer team). -- If it's a major change then it's ok to request a second review - either the author or reviewer can do this. -- PRs that have approving reviews but open questions can be merged by the author after addressing the questions to a common-sense level of satisfaction. -- If the reviewer feels uneasy about this then they shouldn't leave an approval, but rather a request-changes. +- Authors/Reviewers should merge after one positive review and with no obvious open questions. + - Exception in merges to main for releases (two reviews). + - Exception in pseudo PR review for first release (needs review from core or maintainer team). + - If it's a major change then it's ok to request a second review - either the author or reviewer can do this. + - PRs that have approving reviews but open questions can be merged by the author after addressing the questions to a common-sense level of satisfaction. + - If the reviewer feels uneasy about this then they shouldn't leave an approval, but rather a request-changes. + - "request changes" reviews can be dismissed by the PR author if considered out of date and resolved. - We prefer to use merge commits to merge. This is to avoid merge conflicts when multiple people are pulling with overlapping feature branches. -- We prefer verbose commit histories but easy merges. -- Feature branches should be immediately deleted after merge. Ideally by selecting the new feature on GitHub repo settings -- Note: squash merging commits in a PR and merging after is fine, that is up to the individuals. + - We prefer verbose commit histories but easy merges. + - Feature branches should be immediately deleted after merge. Ideally by selecting the new feature on GitHub repo settings + - Note: squashing commits in a PR and merging after is fine, that is up to the individuals. ## Getting help From cba37109f8f470980091f1e5adfa9f5cccbf3aeb Mon Sep 17 00:00:00 2001 From: Lorena Pantano Date: Wed, 9 Oct 2024 16:52:26 -0400 Subject: [PATCH 08/10] add link to github docs --- .../contributing_to_nf-core/contributing_to_pipelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md index d5db878642..057b2079b4 100644 --- a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md +++ b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md @@ -19,7 +19,7 @@ The main steps involved in adding a new nf-core pipeline covered below are: 2. [Contribution overview](#contribution-overview) 3. [Testing](#testing) 4. [Patching bugs](#patching-bugs) -5. [PR review guidelines](##pr-review-guidelines) +5. [PR review guidelines](#pr-review-guidelines) 6. [Getting help](#getting-help) 7. [Pipeline contribution conventions](#pipeline-contribution-conventions) @@ -90,7 +90,7 @@ Only in the unlikely and regretful event of a release happening with a bug. - If it's there is a major change in any type of PR (from modules to pipelines) then it's ok to request a second review - either the author or reviewer can do this. - PRs that have approving reviews but remaining open questions can be merged by the author after addressing the questions to a common-sense level of satisfaction. - If the reviewer feels uneasy about this then they shouldn't leave an approval, but rather a request-changes. - - "request changes" reviews can be dismissed by the PR author if considered out of date and resolved. + - **request changes** reviews can be [dismissed](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/dismissing-a-pull-request-review) by the PR author if considered out of date and resolved. - We prefer to use merge commits to merge. This is to avoid merge conflicts when multiple people are pulling with overlapping feature branches. - We prefer verbose commit histories but easy merges. - Feature branches should be immediately deleted after merge. Ideally by selecting the new feature on GitHub repo settings From f02591994a47fa81f5ab1ca54c863a394a38b0fa Mon Sep 17 00:00:00 2001 From: Lorena Pantano Date: Wed, 9 Oct 2024 16:53:34 -0400 Subject: [PATCH 09/10] adding contribution to PR --- .../contributing_to_nf-core/contributing_to_pipelines.md | 1 + 1 file changed, 1 insertion(+) diff --git a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md index 057b2079b4..efa5750f86 100644 --- a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md +++ b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md @@ -84,6 +84,7 @@ Only in the unlikely and regretful event of a release happening with a bug. ## PR review guidelines - Authors/Reviewers can request help for review at nf-core Slack `#request-review` channel. +- It's fine / we encourage for other people to push to your PR. For example fixing linting (can mention bot commands) but also to build on or fix code if appropriate. - Authors/Reviewers should merge after one positive review and with no obvious open questions. - Exception: merges to master for releases requires **two** reviews. - Exception: pseudo PR reviews for a first release needs **review from core or maintainer team**. From cf64f1b8c121cc2b22208b592c6de208c67a0c41 Mon Sep 17 00:00:00 2001 From: Lorena Pantano Date: Wed, 9 Oct 2024 16:55:56 -0400 Subject: [PATCH 10/10] rephase second point --- .../contributing_to_nf-core/contributing_to_pipelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md index efa5750f86..f1b9bf05af 100644 --- a/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md +++ b/sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md @@ -84,7 +84,7 @@ Only in the unlikely and regretful event of a release happening with a bug. ## PR review guidelines - Authors/Reviewers can request help for review at nf-core Slack `#request-review` channel. -- It's fine / we encourage for other people to push to your PR. For example fixing linting (can mention bot commands) but also to build on or fix code if appropriate. +- We encourage collaboration in PR, other contributors are welcome to fix linting errors (you can mention relevant bot commands) or make improvements to the code if necessary. - Authors/Reviewers should merge after one positive review and with no obvious open questions. - Exception: merges to master for releases requires **two** reviews. - Exception: pseudo PR reviews for a first release needs **review from core or maintainer team**.