Skip to content

Commit

Permalink
Pull Request Etiquette and Best Practices (#5953)
Browse files Browse the repository at this point in the history
  • Loading branch information
donnie-msft authored Oct 4, 2024
1 parent 0cccbcb commit 0bf00d2
Showing 1 changed file with 14 additions and 0 deletions.
14 changes: 14 additions & 0 deletions docs/workflow.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,20 @@ 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

- 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: ".
- 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 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.
- 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.
Expand Down

0 comments on commit 0bf00d2

Please sign in to comment.