Skip to content

pull-requests.md

Latest commit

 

History

History
132 lines (98 loc) · 5.39 KB

pull-requests.md

File metadata and controls

132 lines (98 loc) · 5.39 KB
Raw

Pull requests and code review on RERO+ GitHub projects

This page describes the practice shared by the team for pull requests (PR) and code review.

Principles

  • Parallelize. Do not turn the PR into a queue where each step waits for the previous one to be completely finished.
  • All PRs are not equal. A dependency update or a small localized change does not have to wait for reviews.
  • Do not be too strict. A part of the PRs does not need a review at all.
  • The rest is up to you. Draft mode, waiting for the CodeRabbit review before calling the reviewers or not: everybody organizes these details as they prefer.

The workflow

flowchart TD
    pr(["Create the PR"]) --> self["Review your own code"]
    self --> ready{"Is it really ready<br/>to be reviewed?"}
    ready -->|No| self

    ready -->|Yes| need{"Does it need<br/>a review?"}
    need -->|"No: docs, style, dependency update,<br/>small localized change"| approve
    need -->|Yes| askreview["Call the reviewer you need + the PO<br/>say what to look at"]

    askreview --> address["Address the comments"]
    address --> resolve["Resolve the comments"]
    resolve --> deploy["PO deploys, if necessary"]
    deploy --> potest["PO tests"]
    potest --> fixes["Corrections"]
    fixes --> approve["PO approves<br/>= PO validation"]
    approve --> merge(["Merge by the author"])

    auto["GitHub Actions + CodeRabbit<br/>run in parallel"]
    pr -.-> auto
    auto -.-> askreview
Loading

Step by step

Before opening the PR

  1. Test your PR from scratch locally: bootstrap, setup and test the part of the interface/cli touched by your work.
  2. Make sure your branch is up-to-date with the desired branch (parent branch you come from).
  3. Check your commits, see Commits and what to look at.

Review your own code

If an LLM helped you write the code, you MUST have read and understood what you commit BEFORE calling other reviewers.

Call the reviewers

  • Think about who you really want as a reviewer, and call only that person. In general: 1 person + the PO. Choose them for their knowledge of the part you touched, not out of habit.
  • Call a second reviewer only when you estimate that both really need to look at the PR, for instance when it touches two areas (backend and UI) or when the choice you made concerns them both.
  • Be sure the PR is ready to be reviewed when you call the others. A reviewer should never discover work in progress.
  • Indicate what to look at, see below.

Address, then resolve the comments

Address the comments, then resolve them, so that the PO knows when everyting is OK.

PO tests, approval and merge

If PO tests are needed, the PO deploys (or tests locally) and tests+reviews, then you make the corrections.

The PO approval is the PO validation: it means the change has been tested or approved.

Once the PR is approved, the author does a rebase and merge.

When do you need a review?

There is no strict rule: the point is to spend the team's attention where it is useful. The Conventional Commits type of your change is a good first indicator.

Commit type By default Call a reviewer anyway when
feat review — a new behaviour almost always contains a design choice, a data structure or something the user will see
fix it depends the cause was not obvious, the fix changes the logic in a central place, or you are not sure it is fixed at the right place
refactor, perf review — this is exactly where a human reviewer is the most useful: structure, maintainability, alternatives
build, ci no review it touches the release, publication or deployment pipeline: a mistake there is expensive
test no review it changes shared fixtures or the test infrastructure everybody depends on
chore (dependency updates) no review a major version bump comes with breaking changes to handle
docs, style, revert no review (for a revert, say in the PR why you revert)
breaking change (! / BREAKING CHANGE:) review — always, whatever the type

Whatever the type, ask for a review when:

  • you are not entirely sure about the approach you took;
  • it touches the data model, the schemas, the permissions or another key part of the application;
  • it has an impact on the user experience;
  • the change is large (100+ lines) or spread over many files;
  • a data migration is needed.

And do not ask for one when the change contains no design choice: mechanical or repetitive edits, changes fully covered by the tests, and you are confident about them.

What the human reviewer brings

Linters and CodeRabbit already cover style and small mistakes. As a reviewer, look at what they cannot judge:

  • is there a better way to do this?
  • architecture and data structures;
  • is it maintainable, does it touch key parts of the application?
  • user experience;
  • are the conventions respected, in particular in code written by an AI?

Commits and what to look at

When your PR contains corrections or a refactoring on top of your main change, help the reviewers read it: either split it into two commits, or leave a comment on GitHub to tell what has to be looked at.

At the daily meeting

  • Explain the PRs you opened, and the ones that need attention.