PR&CR

Before you start working on a new feature, you should make sure you're working on a suitable branch. To achieve this, do the following:

1. Switch to the master branch. Make sure you don't have any uncommitted changes that may be in conflict.

   git checkout master
   

2. Pull the latest changes.

   git pull
   

3. Create your branch and switch to it.

git checkout -b <branch-name>

When you have made and committed some changes, you can push them as follows:

git push --set-upstream origin <branch-name>

Note that a simple git push will be sufficient the second time around.

If multiple people are working on the remote branch, you will need to run

git pull --rebase

before pushing.

You should name your branch the name it has on Linear.

When opening your pull request, make sure to give it a descriptive name and a useful description. The latter should either reference the related Issue or describe directly what the intended outcome of the changes is. However, you should also avoid being to verbose. For example, you don't need to describe which parts of the code you changed because that is obvious from looking at the changes.

For every PR, you should add the Head of Software, your PM and another Software team member as a reviewer.

There are three main properties that code reviews aim to ensure for all changes that are being made:

1. Correctness (does the code do what its supposed to?)
2. Functionality (is this a reasonable solution for the problem?)
3. Maintainability (will other HYPED members be able to read and understand the new code?)

As a reviewer, here are the steps you should take before approving a PR:

1. Make sure the code compiles and all tests pass. (This is done by CI, so it's straight forward.)
2. Coding style
   • variables
     • Is it clear what every variable is used for?
     • Are there any unused variables/parameters?
     • Is every variables stylised correctly?
     • Maybe some thing should be written into a variable for readability?
   • types
     • Is the const keyword being used enough?
     • Is the auto keyword used sensibly?
   • grammatical correctness
   • comments
     • too many? redundant? (e.g. x += 2; // Increase x by two)
     • too few?
   • Anything else that makes the code hard to read; clever code isn't always good!
   • Are all includes required?
   • Is the using keyword used? If so, is there any good reason for it?
3. Programming errors
   • If the reviewer can't verify the correctness of the code by themselves, it's bad code.
   • Integration with other subsystems. Are any invariants violated?
   • Are compile time guarantees being made use of enough?
4. Testing
   • Have a sufficient number of tests been written?
   • If a bug was fixed, has a test been added to catch something similar in the future?
   • Tests should never include the same logic as the code itself.
5. Documentation
   • Does it exist?
   • Is it accessible?
   • Is it correct?
   • Is it useful?

Don't hesitate to point things out!

If they don't respond to the requested changes, ping the author of the PR on Discord to let them know.