Skip to content

How to review a pull request

Jump to bottom
Dan Lawrence edited this page Feb 14, 2023 · 3 revisions

Reviewing Pull Request Guidelines

So, you want to be a PR reviewer?

Reviewing a PR can be a daunting task - so much responsibility! However as with most tricky things once you break it down into smaller chunks it’s not so bad, and don’t forget, if you mess up reviewing it can always be fixed in another PR. The ultimate PR reviewers are the thousands of users out in userland, they got you.

The Jargon

  • PR = Pull request. How people start the process of contributing code to pygame_ce.
  • LGTM = Looks good to me. A frequently used phrase when you have finished reviewing a pull request. I like to add a cheeky thumbs up too.
  • CI = Continuous Integration. Mostly a bunch of test runners that will run the pygame-ce tests on a large range of hardware and software versions. Also lints your code style.
  • Linter/Autoformatter = Generally enforces the python community coding style standards known as PEP 8 on python code and Clang coding standards for C. pygame_ce uses the black autoformatter for python and clang-format for C code.

A reviewer's checklist:

  • Does it build & run? First step is to grab the PR branch on your local machine. I do this with PyCharm using Git->Manage Remotes:

    image

    To add the repository of the person who submitted the PR, then a quick ‘Fetch’ from the same menu then I can select the branch from the branch selector in the bottom right of the main PyCharm window. Once you have the branch, a pip install . -U command is usually enough to build it. Assuming it builds I then I run the tests for whatever module has been changed to check they run.

  • Does it do what it says on the tin? Most PRs claim to fix a bug or add a new feature - does this bug appear to be solved, does the feature work? I usually create a quick pygame_ce program and try it out if it seems reasonable to check - or if the contributor has provided a test program I run that.

  • It’s Code eye-ballin’ time. Having looked at it like a user and checked everything is in order, it’s time to look at the actual code. This bit you get better at with time but;

    • Does the new code look like the other code around it? Are there any patterns that look similar but now there is a missing bit in the new code?
    • Leaking memory - any objects created on the heap, but not destroyed or passed out of their creating function?
    • Sensible names. Are the variables and functions given understandable names that make it clear what they do?
    • Tests, docs and hints - the CI will enforce some of this, but if there is new functionality a test will help keep it working over time and documentation will make it understandable and usable to users of the library.

Filtering the PR list

A long PR list got you down? Can’t remember which PRs need your reviewing attention? Well, luckily you can trim that thing right down with some handy filters. Personally I like:

is:pr is:open -reviewed-by:@me draft:false -author:your_GitHub_name

As a good opener - that clears any PRs you’ve already reviewed, any drafts that aren’t ready for review and any PRs that you have made yourself.

Clone this wiki locally