Development Process¶
Note
What follows is a description of what we consider a “pretty good” development process. We strive to achieve it but we may never fully do so. This is not a strict list that you must follow down to the letter it’s more of a high level overview and recommendations how the process should be structured. If unsure about any of the steps the best practice is to use common sense.
Pull Requests (PRs)¶
As mentioned in Git branching and merging workflow we do most of our work in topic branches. Once you are happy with your work (see Definition of Done) you must go through a pull request before you can merge your work into master.
Pull requests are the place and time for colleagues to get familiar with your work and provide feedback. This ensures that we all are on the same page. A PR is the perfect place to test if your code is readable and understandable by others. For example, if a lot of the reviewers don’t understand part of the code then this part should either be rewritten or more comments need to be added.
Before Opening a PR¶
Before opening the pull request, take some time to get the git history and the commit messages in order. Most often this would mean you’ll need to reorder and combine (squash) commits which are logically a single change but have been recorded into multiple, not necessarily consecutive commits. Your commit messages will also be under review so they should be as close to their final version as possible.
git rebase -i is your friend here. It allows you to edit the commits in a number of ways - reordering them, editing their messages, splitting a commit into two or merging two into one. Creating a nice history is important. Take your time to learn how to use interactive rebases.
Opening a PR¶
PRs are a github thing. They can be opened using the github cli but the recommended way is to use the github website. It’s easier and offers more functionality. When opening a PR you will be asked two things: a description and a list of reviewers.
It’s very important to write a good description. Reviewers will normally read it before looking at the diff, so make sure it gives them enough context so they know what they are looking at.
Here is an example of how to structure your PR description. It’s recommended to change and adapt this structure to the specific change you’re making, including changing/adding/removing headings.
## Background
Provide enough information for people to have context for your changes.
What problems the code is solving now, why have we picked this solution historically, etc.
## Aim
Provide a short summary of the changes. Try to include answers to the **Why?** questions,
e.g. **Why are we introducing this change?**, **Why now?**, etc.
If your changes make UI changes, consider providing before and after screenshots.
You can also include a video demonstrating the old vs. the new UX.
## Implementation
Explain why things are done the way they are in this PR.
Highlight the most important and/or controversial design decisions you have taken.
**Why it's implemented the way it is? What alternative implementations have you
considered and why you chose this one?**
The description is a good place to include questions that came up during development like
**Is this name the best one?**, *Could approach B be more applicable in this case?*, etc.
This is also a good place to talk about performance and security considerations if any.
After specifying a description you have to specify the list of reviewers. There is a trade-off here. The more reviewers you specify the better we will understand what each of us is doing but PRs will require more time. After some deliberation we decided to keep the number of reviewers to a minimum (i.e. just one) in order to keep things relatively speedy and have a clear idea who is responsible for the review. The reviewer should be in most cases a more senior developer. Additional reviewers may be included if you feel necessary. These may include:
Anybody who was involved in the PR’s development
Anybody who might be interested by the PR somehow (e.g. people responsible for other parts of the system)
Any additional stakeholders
Addressing Review feedback¶
When addressing the feedback during Code Review, you might be tempted to commit everything in a single “Code Review comments” commit (or something similar). Don’t. Instead, do a single commit for each logically separate change.
Use Git’s “fixup” option to link the new commit with the original one.
git commit --fixup=HEAD^^
Before merging into master, you should squash all “fixup” commits.
git rebase -i --autosquash master
After addressing a reviewer comment, write a reply to the reviewer. Something simple like “Fixed” is enough. The goal is for the reviewer to be notified of the changes and review them again. If the reviewer is happy they will close the thread by clicking the “Resolve Conversation” button.
Closing the PR¶
Once all comments have been addressed and all reviewers have accepted the changes the topic branch can be merged into master. It is advisable to use the git cli to make the actual merge and not rely on the buttons on the GitHub PR page. Once you push the new changes to master GitHub will notice this and automatically mark the PR as merged.
Don’t forget however to delete the remote branch in github in order to keep the number of branches managable. GitHub should display a large “Delete Branch” button at the end of the PR page. Additionaly you make also want to delete your local branch so it doesn’t get in the way anymore:
git branch -d <branch-name>
Definition of Done¶
We consider a task done when atleast the following criteria have been met (there may be more depending on the task):
The code has been commited in a series of logical commits with good commit messages
The code is accompanied by tests that provide good coverage (preferably 80% or above)
The code fully passes the style guide as defined by flake8 and pylint
All relevant documentation has been updated
New documentation (if needed) as been added
Everything is green in Jenkins.
A Pull Request has been opened, all review feedback addressed and all reviewers have accepted the changes.
The PR has been merged.
Some Common Pitfalls and Problems with PRs¶
The development process slows down. When using PRs (or any other sort of code review) we trade speed for quality. We move slower but the quality of the code is supposed to increase. However this may lead to some frustration as people are following this process where previously there was no process. You may be blocked waiting for a particular reviewer and this may be frustrating. You have to be prepared for this. It is part of the process unfortunatelly. As we use PRs more and more, we will get faster and faster but it will never be the speed and freedom of commiting directly to master.
Have a realistic expection about review times. If you open a PR now don’t expect it will be reviewed in an hour. It may take a few hours or even a day to do so. If a reviewer is taking longer than a day then feel free to contact them and nudge them.
The communication in a PR is asyncronous meaning you shouldn’t expect an answear to any comments immediatelly. Again, it may take time. If something is very important it is best to contact the reviewer(s) directly via Zulip.
PRs introduce a new activity that we should do daily. In addition to coding and meetings now we also do pull request reviews. You may pick a specific timeslot and do PR reviews everday at that time. For example, let’s say everyday at 15:00 you do PRs. This helps turn the review process into a habit.
Pull Request Reviews Hurt! The first time you open a PR you may get angry at the comments and think to youself “Do they think I am stupid! They shouldn’t tell me how to write code.”. No one thinks anyone else is stupid. People are just trying to help. They try to share their experience and knowledge. It is normal to get angry but know that everybody is just trying to help.
