From b57ed74cc3eaaa170c8b12c7c1df5b59f5a00b49 Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Tue, 31 Oct 2023 19:57:15 +0100 Subject: [PATCH 1/2] CONTRIBUTING: offer clearer guidelines about blocker vs. non-blocker issues in a PR Brought by https://matrix.to/#/!kjdutkOsheZdjqYmqp:nixos.org/$IuFJ-rB798l9jYfcWm9RyYfD5EDb420xc0kwa-FgZZo?via=nixos.org&via=matrix.org&via=nixos.dev discussion. We sometimes have reviews that are blocked because of miscommunication between author and reviewer, we make this clear so that reviewers and authors can refer to a block of "authoritative" text to unblock their own situations. --- CONTRIBUTING.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 06b9c10dfec6f..71d3667e40895 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -316,6 +316,12 @@ When reviewing a pull request, please always be nice and polite. Controversial c GitHub provides reactions as a simple and quick way to provide feedback to pull requests or any comments. The thumb-down reaction should be used with care and if possible accompanied with some explanation so the submitter has directions to improve their contribution. +When doing a review, consider focusing on the changes proposed, if you find out about anything that could be improved but is not part of the proposed changes, consider tracking it as an issue, **offer** the original contributor to do a follow-up pull request or do it in that pull request, but remember that you should ideally not make this a blocker or give the impression to the contributor this would be a blocker for this merge. If the follow-up change is structural for the contribution, make your point clear about why you feel it is important to do it before the proposed changes. + +Also, consider giving to the contributor a clear representation of what you consider to be blocker and not to help them prioritize their efforts towards a potential merge. + +Examples of follow-up changes can involve a cleanup in the touched files. + Pull request reviews should include a list of what has been reviewed in a comment, so other reviewers and mergers can know the state of the review. All the review template samples provided in this section are generic and meant as examples. Their usage is optional and the reviewer is free to adapt them to their liking. From 038313080b66246b5d2b2f3fa53b8b7d2fd97273 Mon Sep 17 00:00:00 2001 From: Valentin Gagarin Date: Tue, 4 Jun 2024 09:48:43 +0200 Subject: [PATCH 2/2] reword for clarity --- CONTRIBUTING.md | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 71d3667e40895..0e32227bd1579 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -316,11 +316,21 @@ When reviewing a pull request, please always be nice and polite. Controversial c GitHub provides reactions as a simple and quick way to provide feedback to pull requests or any comments. The thumb-down reaction should be used with care and if possible accompanied with some explanation so the submitter has directions to improve their contribution. -When doing a review, consider focusing on the changes proposed, if you find out about anything that could be improved but is not part of the proposed changes, consider tracking it as an issue, **offer** the original contributor to do a follow-up pull request or do it in that pull request, but remember that you should ideally not make this a blocker or give the impression to the contributor this would be a blocker for this merge. If the follow-up change is structural for the contribution, make your point clear about why you feel it is important to do it before the proposed changes. +When doing a review: +- Aim to drive the proposal to a timely conclusion. +- Focus on the proposed changes to keep the scope of the discussion narrow. +- Help the contributor prioritise their efforts towards getting their change merged. -Also, consider giving to the contributor a clear representation of what you consider to be blocker and not to help them prioritize their efforts towards a potential merge. +If you find anything related that could be improved but is not immediately required for acceptance, consider +- Implementing the changes yourself in a follow-up pull request (and request review from the person who inspired you) +- Tracking your idea in an issue +- Offering the original contributor to review a follow-up pull request +- Making concrete [suggestions](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request) in the same pull request. -Examples of follow-up changes can involve a cleanup in the touched files. +For example, follow-up changes could involve refactoring code in the affected files. + +But please remember not to make such additional considerations a blocker, and communicate that to the contributor, for example by following the [conventional comments](https://conventionalcomments.org/) pattern. +If the related change is essential for the contribution at hand, make clear why you think it is important to address that first. Pull request reviews should include a list of what has been reviewed in a comment, so other reviewers and mergers can know the state of the review.