Skip to main content

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.

tip

If you're a new team member, we encourage you to review PRs as soon as you feel comfortable doing so! In addition to benefitting the project, it's a great way to learn more about the codebase and project in general. Consider starting with areas you're most familiar with, and work your way up to more unfamiliar and complex PRs as you gain experience.

If you're ever unsure on any part of PR reviews, don't hesitate to loop in a team member 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​

note

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:

Add the awaiting response label to a PR and remove 1 approval if it exists whenever you post a request for changes. 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.

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

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: unionConstituents and intersectionConstituents).
  • Unit tests:
    • 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

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.

tip

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!

Approvals​

If the PR addresses a time critical task, such as a security fix or main branch build break, go ahead and squash merge it.

Otherwise, upon completing your review, if the build is passing and you have no blockers, approve it on GitHub. Then:

  • If there isn't a 1 approval label or existing approval, add the 1 approval label
  • If there's already 1 approval and/or it's been a week since the last request for changes, go ahead and squash merge
    • For straightforward PRs that don't impact users, you can wait 3 days instead

There's no need to reset waiting periods for minor fixups from code reviews of otherwise approved code.

We try to leave PRs open for at least a week to give reviewers who aren't active every day a chance to get to it.

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:

  1. Ping the author with a message like the Checking In template
  2. Add the stale label to the PR
  3. Wait 2 weeks
  4. 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:

If you have time, consider occasionally looking through old PRs to make sure there aren't any questions left unanswered for weeks.