-
Notifications
You must be signed in to change notification settings - Fork 89
PR Review Guidelines
This document aims to provide a set of guidelines to encourage developers to submit more pull request (PR) reviews. Adhering to the guidelines will improve the consistency and quality of our code and make sure we are all on the same page.
These are typically straightforward and only require approval before merging.
- When a check is failing:
- If it's something impacted by your changes, please address the failure in order to help the PR move along
- If it's something complicated, needs more time or is related to test instability, comment on the PR about any findings and bring this up in the core channel, where we will either
- Provide assistance, or
- Get confirmation to create a follow-up issue
- Review and approve if everything looks good from an editorial perspective
- assign the
Esri/calcite-pesgroup to review and ensure doc tone consistency
- Make sure to apply the
pr ready for screenshot testslabel- If there are additional visual changes since the last screenshot test run, this label will need to be removed then added again to schedule another test run
- Assign the
Esri/calcite-designersgroup to review screenshot diffs, and optionally mention them to increase visibility (especially for design-driven updates) - Wait for design review prior to accepting new or updated screenshot diffs. Obvious false positives can be accepted prior to design review.
- Tag the
Esri/calcite-pesgroup in a comment so a related issue can be created, estimated, and prioritized for an upcoming milestone - Provide context and details for the new issue(s) to better we prepare ourselves for future follow-up work
- Reach out in the Core - General Teams channel about this and cc @jcfranco.
When a pull request is strictly intended to deprecate an existing feature or component, use the deprecate commit type. This type is reserved for noting changes under the "Deprecations" section of the changelog.
If deprecations are part of a broader change, such as introducing a new feat or making a fix, a deprecate entry can be added to the bottom of the PR body following the format described in the release-please documentation.
PR title:
feat(avatar): add in an awesome new feature ✨
Description:
This PR adds in feature x which will make everything work flawlessly forever.
deprecate(avatar): deprecate old unnecessary property 🗑️
deprecate entry at the bottom of the PR body as shown above only when the PR is not already scoped as a deprecation.
- In addition to requesting a review on GitHub, the quickest way to get attention for your PR is to post a message in the Core - Development Teams channel.
- Before starting a review on a non-draft PR (see Draft PRs below), use the 👀 reaction to let others know the PR is being reviewed.
- If the review was requested via a Teams message, use one of the following reactions to indicate status:
- 👀 reviewing
- ✅ approved
- 🛑 changes requested
- 💬 commented
Draft PRs are intended for iteration and refinement before formal team review.
PRs in draft state should not be reviewed unless explicitly requested. This helps avoid spending time on code that is still evolving.
Before marking a PR as ready for review:
- Run an initial review with GitHub Copilot to catch obvious issues or improvement opportunities
- Address relevant CI failures
- Ensure the PR description clearly explains intent and scope
- If there are visual changes, ensure that the
pr ready for visual snapshotslabel is added
Important
Use your best judgment when reviewing Copilot comments. Some suggestions may not align with our tech stack, best practices, or conventions.
When commenting, it is crucial to provide the (PR) author with the priority/context of each comment, e.g., what must be changed, what's a nice-to-have, etc.
Use the following syntax to shed light on the type of comment:
-
Nit: minor adjustments that shouldn't prevent merging -
Suggestion: alternative approach that shouldn't prevent merging -
Sidenote: a relevant note that is not critical to the reviewed PR (should either stem a discussion or an issue/PR to follow-up) -
Typo: address typographical errors (these should be fixed before merging)
Be as specific as possible in your comments to offer reasoning and avoid misinterpretation.
If there is a lot of back-and-forth between the reviewer and author, reach out via chat or schedule a call to make sure everyone is on the same page. Once everything is resolved, make sure to summarize the discussion points and resolution in the PR before moving forward.
Focus on the following items while reviewing:
- Consistency (naming, patterns, etc.) 📚
- Adhering to guidelines 🤝📚
- Improvements or alternatives to a solution, if applicable
- Typos and grammar
- PR title adheres to conventional commits
- Ensure all files are relevant to the associated issue
- Ask clarifying questions about changes if you don't understand something. This ensures knowledge sharing and helps identify information that might be useful to add as comments or rename variables/functions.
You can approve a review if there are a couple required changes, as long as they are straightforward. However, make sure to "request changes" if the changes require a second pass from you.
Be explicit in your recommendation of approval. Communicate if a PR is good to merge once comments are addressed or if a follow-up review request should be made prior to merging.
Getting extra pairs of eyes on reviews is always a good idea, but not necessary. If you'd like to get someone else to take a look, please mention them or add them as reviewers. Similarly, if you are unfamiliar with the changes, please call this out to give a better idea on whether additional reviewers may be needed. If you're unsure on who to contact, refer to the Areas of Focus page for guidance.