From 4092d307719c96a2d15ef1b381f439f752427168 Mon Sep 17 00:00:00 2001 From: Donnie Goodson <49205731+donnie-msft@users.noreply.github.com> Date: Mon, 5 Aug 2024 21:18:14 -0700 Subject: [PATCH 1/5] Pull Request Etiquette and Best Practices (#5950) --- docs/workflow.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/docs/workflow.md b/docs/workflow.md index 853f4e56d55..2267c01d1b6 100644 --- a/docs/workflow.md +++ b/docs/workflow.md @@ -21,6 +21,24 @@ This repo has bots that manage all stale PRs. Stale PRs will be autoclosed. - *Do* use GitHub's tooling. Re-request review after all feedback has been addressed. - *Do* pay special attention to the commit message. Ensure the merge message is appropriate and helpful to the future reader. See [merge commit considerations](#merge-commit-considerations). +#### Pull Request Etiquette and Best Practices + +- If you disagree with the overall approach of the PR, comment on the general PR rather than individual lines of code. +- Leaving [suggested changes](https://docs.github.com/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#adding-line-comments-to-a-pull-request) is welcomed, but please never commit them for a PR you did not create. +- When you are seeking to understand something rather than request corrections, it is suggested to use language such as "I'm curious ..." as a prefix to comments. +- For comments that are just optional suggestions or are explicitly non-blocking, prefix them with "nit: " or "non-blocking: ". +- Avoid marking a PR as "Request Changes" ![2022_01_27_08_33_07_Changes_for_discussion_to_the_PR_Template_by_christothes_Pull_Request_26631_](https://user-images.githubusercontent.com/1279263/151379844-b9babb22-b0fe-4b9c-b749-eb7488a38d84.png) unless you have serious concerns that should block the PR from merging. +- When to mark a PR as "Approved" + - You feel confident that the code meets a high quality bar, has adequate test coverage, is ready to merge. + - You have left comments that are uncontroversial and there is a shared understanding with the author that the comments can be addressed or resolved prior to being merged without significant discussion or significant change to the design or approach. +- When to leave comments without approval + - You do not feel confident that your review alone is sufficient to call the PR ready to merge. + - You have feedback that may require detailed discussion or may indicate a need to change the current design or approach in a non-trivial way. +- When to mark a PR as "Request Changes" + - You have significant concerns that must be addressed before this PR should be merged such as unintentional breaking changes, security issues, or potential data loss. + + + #### Merge commit considerations GitHub merges have 2 means to specify a commit message when squash merging. Inspect both! In most scenarios, you will want to delete the commit by commit messages. Only leave the messages when they are helpful to a user in the future. From 1f28ce3a6894e04db985dc0da504de57d7ca1fca Mon Sep 17 00:00:00 2001 From: Donnie Goodson <49205731+donnie-msft@users.noreply.github.com> Date: Mon, 5 Aug 2024 21:19:48 -0700 Subject: [PATCH 2/5] Address feedback --- docs/workflow.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/docs/workflow.md b/docs/workflow.md index 2267c01d1b6..c49e39d13c4 100644 --- a/docs/workflow.md +++ b/docs/workflow.md @@ -27,7 +27,6 @@ This repo has bots that manage all stale PRs. Stale PRs will be autoclosed. - Leaving [suggested changes](https://docs.github.com/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#adding-line-comments-to-a-pull-request) is welcomed, but please never commit them for a PR you did not create. - When you are seeking to understand something rather than request corrections, it is suggested to use language such as "I'm curious ..." as a prefix to comments. - For comments that are just optional suggestions or are explicitly non-blocking, prefix them with "nit: " or "non-blocking: ". -- Avoid marking a PR as "Request Changes" ![2022_01_27_08_33_07_Changes_for_discussion_to_the_PR_Template_by_christothes_Pull_Request_26631_](https://user-images.githubusercontent.com/1279263/151379844-b9babb22-b0fe-4b9c-b749-eb7488a38d84.png) unless you have serious concerns that should block the PR from merging. - When to mark a PR as "Approved" - You feel confident that the code meets a high quality bar, has adequate test coverage, is ready to merge. - You have left comments that are uncontroversial and there is a shared understanding with the author that the comments can be addressed or resolved prior to being merged without significant discussion or significant change to the design or approach. @@ -37,8 +36,6 @@ This repo has bots that manage all stale PRs. Stale PRs will be autoclosed. - When to mark a PR as "Request Changes" - You have significant concerns that must be addressed before this PR should be merged such as unintentional breaking changes, security issues, or potential data loss. - - #### Merge commit considerations GitHub merges have 2 means to specify a commit message when squash merging. Inspect both! In most scenarios, you will want to delete the commit by commit messages. Only leave the messages when they are helpful to a user in the future. From 49b5f288fd47ed4efea5cf5e90a598da52c344ba Mon Sep 17 00:00:00 2001 From: Donnie Goodson <49205731+donnie-msft@users.noreply.github.com> Date: Thu, 8 Aug 2024 16:03:05 -0700 Subject: [PATCH 3/5] Address feedback --- docs/workflow.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/workflow.md b/docs/workflow.md index c49e39d13c4..003559da5f5 100644 --- a/docs/workflow.md +++ b/docs/workflow.md @@ -25,7 +25,7 @@ This repo has bots that manage all stale PRs. Stale PRs will be autoclosed. - If you disagree with the overall approach of the PR, comment on the general PR rather than individual lines of code. - Leaving [suggested changes](https://docs.github.com/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#adding-line-comments-to-a-pull-request) is welcomed, but please never commit them for a PR you did not create. -- When you are seeking to understand something rather than request corrections, it is suggested to use language such as "I'm curious ..." as a prefix to comments. +- Make sure your intent is clear. Is your comment about just trying to understand the code? Use softer language, such as "I'm curious ...". Do you have a knowledge that the change could introduce a regression? Make your comment emphasize that! - For comments that are just optional suggestions or are explicitly non-blocking, prefix them with "nit: " or "non-blocking: ". - When to mark a PR as "Approved" - You feel confident that the code meets a high quality bar, has adequate test coverage, is ready to merge. From 588054bdcb4ead3dd3790a546a8c3eb32ab60868 Mon Sep 17 00:00:00 2001 From: Donnie Goodson <49205731+donnie-msft@users.noreply.github.com> Date: Thu, 8 Aug 2024 16:03:25 -0700 Subject: [PATCH 4/5] Address feedback --- docs/workflow.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/workflow.md b/docs/workflow.md index 003559da5f5..b3a2e829242 100644 --- a/docs/workflow.md +++ b/docs/workflow.md @@ -23,7 +23,6 @@ This repo has bots that manage all stale PRs. Stale PRs will be autoclosed. #### Pull Request Etiquette and Best Practices -- If you disagree with the overall approach of the PR, comment on the general PR rather than individual lines of code. - Leaving [suggested changes](https://docs.github.com/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#adding-line-comments-to-a-pull-request) is welcomed, but please never commit them for a PR you did not create. - Make sure your intent is clear. Is your comment about just trying to understand the code? Use softer language, such as "I'm curious ...". Do you have a knowledge that the change could introduce a regression? Make your comment emphasize that! - For comments that are just optional suggestions or are explicitly non-blocking, prefix them with "nit: " or "non-blocking: ". From 92688981094adc648f9ddf03050d58737f141ad2 Mon Sep 17 00:00:00 2001 From: Donnie Goodson <49205731+donnie-msft@users.noreply.github.com> Date: Fri, 9 Aug 2024 09:53:53 -0700 Subject: [PATCH 5/5] Address feedback Co-authored-by: Nikolche Kolev --- docs/workflow.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/workflow.md b/docs/workflow.md index b3a2e829242..4fb57657743 100644 --- a/docs/workflow.md +++ b/docs/workflow.md @@ -28,7 +28,7 @@ This repo has bots that manage all stale PRs. Stale PRs will be autoclosed. - For comments that are just optional suggestions or are explicitly non-blocking, prefix them with "nit: " or "non-blocking: ". - When to mark a PR as "Approved" - You feel confident that the code meets a high quality bar, has adequate test coverage, is ready to merge. - - You have left comments that are uncontroversial and there is a shared understanding with the author that the comments can be addressed or resolved prior to being merged without significant discussion or significant change to the design or approach. + - You have left comments that are uncontroversial and there is a shared understanding with the author that the comments can be addressed or resolved either within this PR or a follow up PR, without a significant change to the design or approach. If the author pushes a new change addressing those comments, you may need to re-approve as it's required that the latest iteration of a PR is approved. - When to leave comments without approval - You do not feel confident that your review alone is sufficient to call the PR ready to merge. - You have feedback that may require detailed discussion or may indicate a need to change the current design or approach in a non-trivial way.