PR review guidelines
Edit

Best practices to review contributions.

PR review guidelines

The key words “MUST”, “MUST NOT”, “SHOULD”, etc. are to be interpreted as described in RFC 2119.

Authors/Reviewers SHOULD request help for review at nf-core Slack #request-review channel.
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.

Pull requests to master for releases MUST receive two positive reviews before merging.
(Pseudo) pull requests to master for first releases MUST receive at least one review from core or maintainer team.

If there is a major change in any type of PR during review (from modules to pipelines) then developers SHOULD request a second review - either the author or reviewer can do this.
If the reviewer has strong reservations about proposed changes, a reviewer SHOULD NOT leave only a comment or approval, but SHOULD leave a request-changes review.

Request changes reviews MAY be dismissed by the PR author if considered out of date and resolved.
PRs that have approving reviews but remaining open questions MAY be merged by the author after addressing the questions to a common-sense level of satisfaction.
If a PR has comments from one reviewer with open questions, and they request to re-review but does not perform the re-review, the PR MAY be merged if the first reviewer does not finish the review after 3 months if the PR gets an independent approval from someone else.

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: squashing commits in a PR and merging after is fine, that is up to the individuals.