Reviewing with PullRequest

PullRequest Review 101

PullRequest reviews will appear to the code author(s) as inline and pull/merge request summary comments - just like you would see from another team member.

Think of yourself, the reviewer, as an extension of their development team at the code review level. In cases where you're reviewing for a new team or repository on PullRequest, the best practice approach each code review as if you've recently taken professional ownership of the repository.

Be sure to thoroughly dive into the codebase with the tools you have available and take the time you need to provide a great review. Remember that PullRequest customers love the service because contributions from PullRequest engineers goes beyond things that are surface-level or "linter-like."

Here are some code reviews that have been completed by our reviewers on public repositories:

• ergo by Ought Inc.

• unity-cache-server by Unity

• apps-android-commons by Wikimedia Commons

Example:

How long will I be assigned a code review?

Code reviews remain assigned until the code changes the client has made are merged or closed. This provides an opportunity to maintain a dialog with the code author(s).

If you come across a potential issue, but need a little more info, feel free to ask questions or for clarification.

Example:

The average length of time a code review remains active varies from team to team and the scope of the proposed code changes. This can be a few hours or even a few days. In some cases, a team's ability to merge will depend on PullRequest's review. If that's the case, we'll let you know via prompt when you start working on it.

What kinds of things should I be reviewing for?

Think of reviewing a customer's code on PullRequest as being brought in as a technical consultant.

We're highly selective of the engineers we admit into the network because reviewing code requires great judgment and extensive development career mileage.

Our customers expect thorough review for high-impact items such as: security vulnerabilities, performance issues, opportunities to optimize performance, logical errors, opportunities to reduce technical debt, issues with architecture, opportunities to replace methods/tools with others which are better suited for the task, best practice adherence, tests for edge cases etc.

We also encourage pointing out things that were done especially well. This reinforces good practices and lets the code author know her/his hard work has been acknowledged.

As a general rule: it's always better to call attention to something that may be a concern than not.

Examples:

Handling context

One of the biggest difference between reviewing code on PullRequest and what you're probably used to reviewing for your own team is that you won't have months of on-going familiarity and offline conversations about the codebase. We prioritize available pull requests to review based on reviews you've completed in the past, but from time-to-time, and at first, you'll be new to a codebase.

PullRequest provides a number of tools to help fill in some of these gaps. You'll be able to access the entire repository by means of a (very fast) code search tool, dependency manifests are always accessible as well as any README documentation, and we provide on-going notes for every organization and repository that are visible alongside the pull request description and title.

Teams on the other side of the pull request understand that you don't have all of the context that they do, so we encourage making thoughtful assumptions and prefacing that you can't confirm certain things.

Here's a good example:

Submitting a great review

Code reviews vary depending on a wide range of factors including a team's style of development, the complexity of the proposed changes, the code author's level of experience, the client's business goals, and many, many others. It's important to use good judgment with the context you have.

Here are a few things we strongly encourage:

• Explain the "why" and your thought process - Unless something is very obvious, it's a good practice to elaborate on why you've called attention to an issue. This will help the developers on the other side of the pull request understand your feedback.

• Be generous with code examples and links - Add code snippets and links to sources when and wherever you can alongside issue descriptions.

• Join existing conversations - Other members of the organization may be involved in existing dialogs on a code review. If you have useful information to offer, feel free to join in!

• Follow up on changes - Just like during any code review, it's important to check back up on changes that are made in follow-up commits. You'll be notified when updates are made and we strongly encourage posting a follow-up comment that you've looked at the latest commits - even if they contain no new issues.

Things to AVOID in your reviews

Reviews you submit through PullRequest should NOT be:

• Limited to surface-level code and syntax. In almost all cases the code presented will successfully compile. Your review should focus on if there are more in-depth issues and to make sure the changes are good.

• A "human linter."

• Rushed in any way. Take your time and really dive in.

There are a few things we ask our reviewers to avoid:

• Repeated pattern feedback and nit-picky "to-dos" - While some nitpicks are important for maintainability, like having more descriptive variable names or keeping consistent whitespacing, a slew of inline comments addressing every instance makes for a code review that's hard to read. Depending on the review, these can often be addressed at a high level in a summary comment or leaving one inline comment with the line "ditto throughout."

• Framing feedback as a command - Feedback that's not intended to be a terse command has a dangerous tendency to become perceived as such. Before submitting a review take the time to ensure you're erring on the side of gentleness. Framing a suggestion or feedback as a request accomplishes the same functional goal and is always better received. (Example below)

• LGTM! - While many code reviews won't contain any major items of concern, we encourage our reviewers to add high-level value when possible. The teams we work with look to PullRequest reviewers for guidance and an opportunity to learn and produce better code. This could be things like recommending a more robust or well-maintained library to replace one that's currently being used or calling attention to code that was executed particularly well (as mentioned above).

  • Even if a code review is completely free of issues, your comments should clearly communicate that you've reviewed the change(s) thoroughly.

Remember to use your best judgment

Always remember to put yourself in the position of the developer or team on the other end and consider what information you would find helpful if you were in their situation.

If you ever have any questions always feel free to contact [e-mail] or reach out directly to a PullRequest team member on Slack.