Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pull Request Etiquette and Best Practices #5953

Merged
merged 5 commits into from
Oct 4, 2024
Merged

Pull Request Etiquette and Best Practices #5953

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4092d30
Pull Request Etiquette and Best Practices (#5950)
donnie-msft Aug 6, 2024
1f28ce3
Address feedback
donnie-msft Aug 6, 2024
49b5f28
Address feedback
donnie-msft Aug 8, 2024
588054b
Address feedback
donnie-msft Aug 8, 2024
9268898
Address feedback
donnie-msft Aug 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions docs/workflow.md
View file
Open in desktop
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