Guides for SE student projects »

Guidelines for working with PRs

Selecting what to work on

  • If submitting a PR for an existing issue,
    • It's best for first time contributors to start by fixing an issue specifically labelled for first timers e.g., d.FirstTimers, good first issue.
    • It's best to post in the issue to ask if it is OK for you to submit a PR for that issue and wait for approval.
    • Check the issue discussion thread to see if there are PRs for that issue. You can offer to PR for an issue that has stalled PRs i.e., there is a PRs submitted for that issue but the PR author seems to have stopped working on it.
  • If the issue list does not contain what you want to work on, post an issue first and wait for it to be acknowledged. Otherwise you could end up fixing something that does not need fixing.

Scoping a PR

General rule: try to keep PRs as small as possible because smaller PRs get merged faster.

A PR should contain a single, standalone, and complete change to the code base, unless in exceptional cases where the PR is part of a bigger change.

  • Single means a PR should not try to fix more than one fix, unless there are multiple things that must be done together or not at all.

    Refrain from in the neighboring code unless the line in concern is already touched by the PR (i.e., the housekeeping does not increase the line count of the PR).

    Rationale: Imagine we decide to revert the PR for some reason. If a PR contains, unrelated changes or multiple independent changes, we will not be able to revert the offending change without losing the other changes as well.

    If you notice a need for housekeeping in the neighboring code as you do your PR, create an issue for it in the issue tracker.

  • Standalone means the PR should contain a meaningful change that moves the code base from one working state to another.

  • Complete means the PR should contain everything related to the change, including the following:

    • functional code
    • code comments
    • test cases
    • user docs and developer docs

"This PR is just the code fix. I'll update tests and documentation in a separate PR" is not acceptable!

However, it is fine to push the functional code first to get early feedback, as long as the rest is added to the same PR later.

Submitting a PR

When submitting PRs, follow the forking workflow. A summary of the steps is given below.

Step 0 Do these steps if you haven't done them already:

0.1 Fork the upstream repo.

0.2 Clone the fork to your computer.

0.3 Set up the dev environment as described in the project docs. Confirm the set up is correct.

Step 1 Create a branch from the master branch, following the naming convention given.

Step 2 Add your code to the branch while ensuring you follow these:

  • relevant coding standards (the full list is given here)
  • Commit message format

  • guidelines for commit organization

  • PR scoping guidelines given above

Step 3 Sync your branch with the upstream master, if the master branch advances while you work on your code (i.e., pull upstream master, merge to your branch).

Step 4 Create a PR when the code is ready, as follows:

  1. Run code style checks (if any) to ensure the code complies with the project standards.
  2. Push the branch to your fork.
  3. Create a draft PR from your fork to the upstream repo.
  4. Check the draft PR on GitHub to confirm the following:
    • it follows the PR format conventions
    • it does not contain any unintended changes.
    • it passes checks, if any.
  5. Remove the 'draft' status of the PR. Post a ready for review comment for good measure.

Step 5 Revise as per reviews until the PR is merged.

  • Feel free to post a reminder comment if you don't get a review within 2-3 days.
  • When you receive a review,
    1. Revise the code as per the review.
    2. Push the new code to the branch in your fork.
    3. Post a comment to indicate the PR is ready for a new review.

Reviewing a PR

  • Check for basic PR hygiene, and remind the PR author to rectify if necessary.
    • contains a single, stand-alone, complete fix
    • relevant comments, dev/user docs, tests have been updated
    • PR title/description format is expected
    • doesn't contain unrelated changes

  • Before approving a PR, confirm that all your previous comments have been addressed.

Merging PRs

Follow the convention for Git branch merging, as given in the panel below.