Skip to main content

Pull Requests

Pull Requests (PRs) are the only way code is merged into master/main.

When to open a PR​

  • When work is complete
  • When commits are cleaned up
  • When CI passes

Do not open PRs with:

  • wip: commits
  • Unrelated changes
  • Broken builds / failing CI
Draft Pull Requests
  • Use draft PRs for work in progress to signal that it's not ready for review.
  • Once the work is ready for review, convert the draft PR to a regular PR.

PR description​

Every PR should include:

  • Short summary of what changed
  • Reason for the change
  • Notes for reviewers (non-obvious decisions, trade-offs)
  • References to issues, tickets, or milestones
  • Status of the testing. Describe what testing hase been done and if the test were run in simulation or real-world.

Recommended structure:

Summary

Reason

Notes for reviewers
PR Size and Status
  • Keep PRs small and focused for easier review.
  • If a PR touches many unrelated areas or is hard to review in one sitting -> split it
  • Use draft PRs for work in progress to signal that it's not ready for review.

What to do when the pull request is approved​

The merge is done with one squash merge, which means that all the commits will be in one single commit in the develop branch after the merge. In the Github web interface is called "Squash and merge". When you press the button to do the squash the UI will show a text box with the contents of the commit message. Now it is a good time to fix the commit message and get rid of unnecessary messages ("fix typo", "one more test", ...).

Code review process

Responsibilities of the author​

  • Make the PR easy to review
  • Respond to comments
  • Update code instead of arguing style
  • Keep discussion technical and factual

Responsibilities of the reviewer​

  • Review logic, not just syntax
  • Check:
    • Correctness
    • Readability
    • Test coverage
    • Consistency with existing code
  • Avoid nitpicking unless it improves clarity

Review principles​

  • Comments are suggestions, not attacks
  • Prefer asking questions over giving orders
  • Resolve all comments before merging
  • Approvals mean:
    • Code is understandable
    • Code is acceptable to maintain
    • Code can be merged

CI and checks​

  • CI must pass before merge
  • Failing CI blocks merge
  • Do not bypass checks unless explicitly agreed by reviewer

Typical checks:

  • Tests
  • Linters
  • Static analysis
  • Build verification

Q&A​

Master branch moved forward while the PR was opened, what to do?​

Well, this is simple. Just pull the changes from develop into the feature branch. Do it with merge or rebase (rebase is recomnended).

Why to keep a clean history if the merge will be squashed?​

For the reviewers will be easier to follow and understand changes in the feature branch.