Pull Requests
This document serves as a guide for how you might review pull requests.
Use your best judgement when reviewing PRs, and most of all remember to be kind, friendly, and encouraging when responding to users. Many users are new to open source and/or typed linting. It's imperative we give them a positive, uplifting experience.
If you're ever unsure on any part of PR reviews, don't hesitate to loop in a maintainer who has more context to help!
Governance
Per the scales from Contribution Tiers > Pointing:
- Straightforward: At reviewer discretion, may be merged with a single approval by any committer or maintainer. This includes docs enhancements, bug fixes, and feature additions.
- Non-straightforward: may be merged with either two committer approvals or one maintainer approval. These include multi-package internal refactors and non-breaking public API changes.
- "Unusual"-categorized: require two maintainer approvals. These include significant refactors with cross-project and public API ramifications.
PR Review Flow
We include a set of common responses to PRs in .github/replies.yml
, intended to be used with the Refined Saved Replies extension.
Don't treat these as exact responses you must use: they're just a starting copy+paste helper.
Please do adopt your specific responses to your personal tone and to match the thread for non-straightforward PRs.
Open pull requests ideally switch between two states:
- Ready for review: either newly created or after review is re-requested
- Waiting for author activity: either by virtue of being a draft or having the
awaiting response
label
Add the awaiting response
label to a PR whenever you post a review.
It will be automatically removed if the author re-requests review.
Be Kind
Above all else, be encouraging and friendly in tone.
- Phrase feedback as opportunities for improvement rather than chastising.
- Call out any positive aspects you see - especially in larger pull requests.
- Use happy emojis frequently.
- If you have the energy, post a fun (but not inappropriate) GIF with your review.
- We often use the GIFs for Github extension: available as GIFs for GitHub (Chrome) and GIFs for GitHub (Firefox).
Pull requests are the first time many community members meaningfully interact with us - or, oftentimes, any open source maintainers at all. It's important that we provide a positive, uplifting experience. ❤️
Reviewing a PR
Before reviewing a pull request, try to look back over the backing issue to (re-)familiarize yourself with it. You may find it useful to:
- Attempt to simplify the provided reduction ("code golf")
- Look back through previous issues and PRs around the same area of code / lint rule
If there's no backing issue:
- If the PR is a straightforward docs or tooling fix that doesn't impact users, it's ok to review it as-is
- Otherwise, add the
please fill out the template
label and post a comment like the Needs Issue template
Upon completing your review, if the build is passing and you feel comfortable merging it in, go ahead and squash merge.
Otherwise, add the 1 approval
label.
Common Things to Look For
- Style: that can you read through the code easily, there are comments for necessary tricky areas, and not unnecessary comments otherwise.
- Try not to nitpick things that don't matter.
- If something is standard in the repo but not enforced, consider mentioning it as a non-blocking comment and filing an issue to enforce it.
- Thoroughness: does it handle relevant edge cases? Commonly:
- Generics and type parameters (see:
getConstrainedTypeAtLocation
). - Parenthesis and whitespace (see:
getWrappingFixer
). - Unions and intersections (see:
unionTypeParts
andintersectionTypeParts
).
- Generics and type parameters (see:
- Unit tests:
- All lines are covered per the Codecov /
yarn jest path/to/impacted/file --coverage
report. - Both "positive" and "negative" ("valid" and "invalid") cases exist, if reasonably possible to test for.
- Fixes and suggestions, if present, don't remove
//
or/*
comments invalid
lint rule errors include line and column information- Valid syntax edge cases don't cause the rule to crash, even if they cause a type error in TypeScript
- All lines are covered per the Codecov /
Individual Comments
Post about one comment per area of praise note, suggested change, or non-actionable note. It's fine to use multiple ancillary comments to indicate "(also here)" notes, but don't overwhelm with requests.
If you're posting more than a couple types of comments, consider prefixing them with category indicators such as [Docs]
, [Praise]
, [Testing]
, etc.
This helps avoid the review feeling like a huge slog of big requests.
Be clear in each of your comments what you're looking for or saying. Err on the side of providing more information than you think is needed.
Try to default to a questioning tone for points that aren't clear bugs. Encourage authors to think on your suggestions: "I think (xyz), but am not sure - what do you think?".
Preliminary or Repeat Reviews
Sometimes you may want to post a preliminary review, and/or realize later on you missed something in an earlier review. Both are reasonable and fine.
For preliminary reviews, be clear with what scale you're reviewing at: "Reviewing on architecture now, will look in more detail once it's settled".
For repeat reviews, be clear when it's something you missed earlier and/or there's new information. Don't apologize if the missed information was only made clear because of requested changes - this is part of the review process!
Other States
External Blockers
If the PR is blocked on something, such as an external API or discussion in an issue, add the appropriate blocked by *
label.
Explain why in a comment that links to at least a tracking issue for the blocker.
Out-of-Band Questions
Authors sometimes separately ask questions as comments on PRs, sometimes including an @
tag.
Put back the awaiting response
label if you answer the questions.
Don't worry if you miss some of these questions; we prefer going through the formal re-requesting review to make sure the awaiting response
label is removed as needed.
Pruning Old PRs
Every so often, we like to search for open PRs awaiting response
to find ones that might have been forgotten.
Our flow for PRs that have been waiting for >=1 month is:
- Ping the author with a message like the Checking In template
- Add the
stale
label to the PR - Wait 2 weeks
- If they still haven't responded, close the PR with a message like the Pruning Stale PR template
Searching for PRs
Assorted other queries for PRs include:
- All PRs you commented on
- All non-draft, non-
awaiting response
PRs - All non-draft, non-
awaiting response
PRs not blocked or already approved
If you have time, consider occasionally looking through old PRs to make sure there aren't any questions left unanswered for weeks.