Prerequisites
This guide assumes you know the basics of using a command line (also called a terminal). Specifically, it assumes you know how to navigate around the filesystem and run commands. You can learn how to use the command line in this article. If you are on Windows, you should install WSL to use the Bash terminal, since many of the commands you will be using are not available in the default Command Prompt.
It also assumes you know the basics of using Git, such as cloning a repository, adding and committing changes, and pushing your changes to a remote. If you don't, you can learn about that in this short introduction to Git.
Getting Code Into a Repository
Let's suppose you have created a file which you would like to add to a NUbots repository called MakeRobotStand.cpp, which is part of your project to make the robot stand up.
In the terminal, change directory to the location where the repository is cloned.
Select Branch
If you are working within an existing project, then you need to find out which branch the work is on. To list the branches, use the command git branch -r. When you have found the branch you want to push to, swap to it using the following command:
git checkout <branch name>Where <branch name> corresponds to the branch you found in the last step.
If you are not working on an existing project, you need to make a branch to commit your changes to. The format for branch names is your_surname/brief_description_of_project. So if your surname is Hamiltons, and your project is making the robot stand up, a good branch name would be hamiltons/stand_up.
Making a Pull Request
Once you have a set of commits on your branch which you think should be added to the main codebase, you can make a Pull Request (PR). PRs are where users ask the repository to pull their branch into the main branch.
GitHub has documentation about PRs here.
The title of your PR is important. It will determine what the eventual commit message will be that others will read to know what has changed. Title your PR with a description of the changes that it will make. For this example we could use Add a stand-up script. If you need to give more detail about what the code does, put that as a comment. If done properly, your PR title should finish the sentence "When merged, this PR will <TITLE HERE>".
To be approved, you should add reviewers to the pull request. Each reviewer will receive a notification. If you have a project mentor, they should be a reviewer, and at least one other person who knows about that part of the codebase or the problem you are trying to solve. Once the reviewers have approved the changes, the changes can be merged into the main branch.
Code Review
The reviewers will give constructive feedback about your code, by making comments on code snippets from your changes. You can respond to these on GitHub. They can also directly suggest changes to specific sections. Additional commits you make to the branch will update the PR and will reset reviews of the changed sections.
Once all of the comments are resolved, the reviewers have approved the PR, and the build checks have passed (see Formatting and PR Checks), you can Squash and Merge. Squashing combines every commit on the PR into a single commit so that the main branch history is clean and easy to follow. Once the code has been squashed and merged, it is in the codebase and you have succeeded in contributing code to NUbots.
Commit Etiquette
Commits should make minor changes, and the changes in each commit should have a common theme. A general mantra for contributing code is commit often, push once, but this isn't strict at all. Committing often means that the code is built up incrementally. Pushing once means that you don't spam all the other contributors with notifications.
Commit messages should describe what you did, starting with an action verb in the present tense. If you added a file with a new stand script, for example, prefer "Add new stand script" over "Added new stand script" or "Adding new stand script" or "Add script".
Formatting and PR Checks
Before making a pull request, it is important that your code adheres to the code style and formatting rules for the project you are contributing to.
- For code in the NUbots repository, run
./b formatto fix formatting issues before making a commit. - For code in NUsight, run
yarn eslint:fixto fix formatting issues before making a commit. - For code in NUbook, run
yarn formatto fix formatting issues before making a commit.
If your code is in a different repository and it uses C++ or protobuf files, use the .clang-format file in the NUbots repo to format them.
If you push your changes to GitHub and get a message that the build failed due to formatting issues, try running the relevant format commands, commit any changes, and push to GitHub again. This will trigger a new build, which will check the formatting again. You won't be able to merge the PR until the build checks pass.
Work in Progress PRs
If you have an incomplete project you would like feedback on, you can make a draft PR to get your code reviewed. Prepend [WIP] to the PR title when making draft pull requests. Draft PRs cannot be merged, so there is no need to worry about merging in bad code. GitHub has documentation about draft PRs here.
Reviewing Code
To review code on GitHub, go to the relevant PR page and open the Files Changed tab. This will show the diff of the PR.
You can add comments and/or suggestions to one or more lines in the diff. For a single line, move your mouse cursor over the line and click the plus button that appears next to the line number. For multiple lines, click and drag the plus button down, starting on the first line. A Markdown editor will pop up to write your comment or suggestion. To make a code suggestion, click the button that looks like a file icon with ± inside it, and then make your changes to the code snippet that is inserted. Click the Preview tab to preview your changes. When done, you can either leave a single comment or start a review. In a review, all comments you make will be pending until you finish your review, when they will all be posted at once.
Try to be as clear as possible when making comments. The contributor should be able to understand if the comment is asking for clarification, or asking for changes. Be kind and make sure to word your comments constructively. Prefer "we" over "you" when making suggestions for what to do.
When reading the code, if there is anything you find confusing or potentially incorrect, make a comment with all your concerns. For example, if there is a nontrivial block of code without a comment explaining it, you should ask the author to add a comment. Another example is if there is a block of code that has a side effect that the author may not have considered. You should inform the author of such and suggest an alternative if possible.
When reviewing documentation PRs, you should look for factual inaccuracies, omissions of important information, awkward sentences, and typos.
After reading the code and adding your pending comments, click Finish your review at the top right. This will show a popup with an input for overall comments, as well as the three actions you can take to finish your review:
- Request changes: this blocks the code from being merged until you subsequently approve the PR in another review, even if others have approved it. Your review may be dismissed, but this is generally discouraged.
- Approve: this allows the PR to be merged, so don't do this unless you are reasonably confident that the changes are correct.
- Comment: this adds your comments without blocking or approving the changes. Do this if you are not too concerned about the comments you have made, or if you're not sure when you'll be able to approve the PR once the changes are made.
Reviewing code that you may not be fully confident with and leaving comments asking for clarification helps you learn and helps the author write more readable code, which is better for everyone.
Writing Reviewable Code
Writing reviewable code is a skill that can be developed with practice. Here are some general guidelines to make your PR easier to review:
- Make it atomic: A reviewable PR should only address one issue or add one feature.
- Keep it small: Ideally, a reviewable PR should not change much more than 400 lines of code, anything above this super-exponentially increases the time it takes to review, and makes it more likely that an error will be missed.
- Name it well: A reviewable PR should be named starting with a verb saying what you did, what issue was fixed, or what feature added. Prefer the present tense, e.g. "Update dependency X to version Y" instead of "Updated dependency X to version Y".
To keep PRs small and atomic you may have to split your PR into multiple sub PRs. To do this, make a new branch branch from the original PR and move over some code. Then make a separate PR targeting the original PR's branch. Make sure to mention the original PR in the new PR's description, using #123, where 123 is the sub PR's number. After completing the sub PR, merge it back into the original PR.
Handling Reviews and Reviewee Etiquette
Here are some tips for handling reviews as the author of a PR:
- When a review comment is left on your PR, you may respond with a reply on GitHub, or with a comment in the code. Prefer comments in the code where appropriate, so in the future others don't have to read through several PRs and can instead just read the code.
- When applying suggested changes, you can go to the Files Changed tab and see the comments with the context of the code. Here you can also batch apply suggestions, so they only take one commit.
- After addressing a review comment, you can click Resolve Conversation to hide the conversation thread and reduce visual clutter. It is the reviewer's responsibility to check that your changes adequately addressed their concerns.
- Don't be alarmed when a reviewer makes over a dozen comments on your PR, just work through them systematically, and don't hesitate to ask for help or clarification if you need more details.
- Finally, when all the comments have been addressed from a review that requested changes, you can request another review by clicking the Re-request review button next to the reviewer's name.
