Proposal: Pull Request review expectations

--- robots: nofollow, noindex tags: shield, proposal --- # Proposal: Pull Request review expectations This covers a suggestion for Pull Request review expectations for the Fluent UI Web engineering team. This doc is motivated by challenges we've seen in `v7` where we have a bunch of contributions from outside the team and Microsoft. However, I think this makes sense to apply to the whole repo. The more we have the same policies and expectations across the repo, the easier it will be for everyone to work together. Thanks to David, Xu, Elizabeth, and Juraj for early reviews and contributions. ## Ultimate Goal Improve our PR review response time in order to improve efficency and reduce the effort required to contribute to Fluent UI React. ## Sub Goals: - Provide a better experience to 3rd party and Microsoft devs contributing to Fluent UI React. Thus encouraging them to come back and contribute more. - Make ourselves more productive by not letting PRs linger. Coming back to a PR after it has been sitting often requires significant mental effort. - Maintain quality of PR reviews. We should not reduce time to turn around by reducing quality of review. - Clarify and discuss expectations so everyone on the team knows what to expect around PR reviews ## Fluent UI Dev Expecations ### Code reviewer - **Code owners review PRs in their area and review or respond to outstanding comments within 2 days** (excluding weekends, holidays, and vacation of course) - If the author addresses or replies to reviewer comments, we expect reviewer to follow up again in a day or two. - The whole PR review is not expected to be completed in 2 days, as sometimes it requires many iterations and changes to reach an acceptable fix - **Assignees of PRs are expected to help move the PRs forward and should not let a PR sit unattended.** - **PRs start assigned to active Shield Dev** and get assigned to primary reviewer (usually code owner) once review is underway - If the codeowner is obviously active on the review, it's ok to just assign the PR to them. Otherwise there **should be positive handoff before a PR is assigned** to someone (to avoid PRs bouncing around the team) - If you are assigned a PR that you are not actively working on, you should be working to find the right person to assign it to (with a positive hand off, not blind re-assignment) - This probably means that **everyone needs to check github notifications on most days and account for time reviewing others' active PRs** - This does not propose a significant increase in time spent on PRs. It is proposing shorter turnaround times, which should actually reduce overall time spent on PRs in the long run. - Two options to monitor are: - GitHub Pulls [Assigned Pull Requests](https://github.com/pulls/assigned) and [Review Requests](https://github.com/pulls/review-requested) - GitHub Notifications [Mentions](https://github.com/notifications?query=reason%3Amention) and [Review Requests](https://github.com/notifications?query=reason%3Areview-requested) - Spending this time to check on mentions will also help turnaround time on issue follow up - Responding quickly to issues and PRs should have a net benefit to productivity for the team. Even if it requires additional effort from individuals. - We can likely also use github bot rules or other automation to make it easier to remind ourselves we need to follow up. ### Code author - If you need attention from code reviewers, you can do one (or multiple) of the following to get attention (listed in preferred order): - Click *re-request review* button next to the reviewer's name. - At mention reviewer's name in a comment. (e.g. "@foo, all comments are resolved. could you take another look at my PR?") - Directly ping the reviewer on Teams (if you are a MSFT employee). - As a courtesy to reviewers, PRs that are **not under active development or not ready for review should be converted to draft or closed.** - Draft PR can be used for demoing code which is not ready to be merged yet, or for getting a full CI build. - PR should be closed if no clear need. - This helps a reviewer know if they are going to be expected to follow up on a PR or if it needs more work. - PRs that are ready to review, but not ready for merge should be labeled `Status: Do Not Merge` - A PR that is passing checks may be approved and merged at any time. - Avoid leaving PRs around that you are not OK with having merged. - The `Needs: Author Feedback` label indicates the reviewer is waiting on author changes or response - This label is added when changes are requested - It may also be added by the reviewer when they have raised comments and are waiting for author feedback - PRs with `Needs: Author Feedback` will be closed after 5 days of no activity (should we change this to 7 days?)