Skip to main content

Pull Requests and Code Review Guide

This is a guide not law.

Use common sense.

Have fun.

Protocol

  1. Open PR from your personal fork to a aquarist-labs repo.
  2. Assign to yourself. Set related Issue if it exists.
  3. Pick a reviewer. Ideally someone with domain knowledge. Pick more than one if the PR is particularly complex.
  4. Wait for review(s). Give about 2 days. If you haven't heard back after that or the PR is urgent ping reviewer(s).
  5. Discuss, address feedback until ≥ 1 reviewer(s) approve. No active reviewer disapproves.
  6. Merge as soon as your work is approved

Review Purpose

Improve quality

Catch bugs

Good engineering

Side effects:

  • Learning opportunities
  • Expose where we as a team have blind spots and more work to do (e.g create issues)

Pull Request Anatomy

Has a topic, description and ≥ 1 commits.

A PR or commit must not contain changes that are not mentioned in a commit message. It does what it says it does.

Topic Which feature, bug fix does the PR address?

Description Use the provided template and checklist. Add issue number if it exists. Summarize your changes.

Commits Each commit contributes to the PR's topic. Some exceptions are okay: Refactoring to make follow ups possible/easier. Minor formatting changes. Individual commits are GPG signed and have a DCO.

Expectations

Role: Reviewer

Answer any questions the author has.

Ask questions to understand the how and why.

Provide productive, constructive feedback.

Point out style violations (if not caught by automated system).

Suggest changes to improve code quality.

Optional:

  • Run tests locally
  • Run benchmarks

Role: Author

Drive the PR towards a merge.

Incorporate feedback received from reviewers.

Drive the discussion.

Requests the review only after tests and static analysis tools pass in our CI. Exception: Draft PRs to receive early feedback.

Suggestions

How to Do Code Reviews Like a Human Blog Series

Size

As small as possible. As big as necessary. Should capture a meaningful change.

Individual Commits

Should have proper messages.

Should tell a story of how the change comes to be.

Learning Opportunities

If you are uncertain about how you expressed an ideal in code or like a suggestion on how to improve code readability. Feel free to ask the reviewer for a suggestion.