-
Notifications
You must be signed in to change notification settings - Fork 1
Code Control
We use GitHub Flow for both branching strategy and code review.
In short:
-
mainis always deployable. Only working code ends up inmain. - No change directly to
main. Create your own branch. - When you want any feedback, create a PR. Leave auto-merge off until you are ready. Som lazy reviewer might approve!
- When others changes are merged to
main, make sure to include them in your branch too!pull origin/mainoften!
All our repos use the same merge settings and policies for the default branch:
- Only "Allow squash merging" is enabled. This is to avoid bloating the history of the default branch. History of importance within a branch should be stored differently than in a commit.
- Auto-merge is allowed. Though use with caution. We need to give team members a chance to see the changes the PR contains and have time to weigh in.
- Merged branches are automatically deleted once the PR and merge is complete. This is to keep the repo and its branches clean. If you are starting new work, start a new branch!
- Name of default branch is
main, in empathy with the social inclusion movement of June 2020.
This forces us to funnel all changes into main through pull requests!
PRs and code reviews is how we stay focused on continuously committing and releasing code, features, and integrations so we deliver even better solutions and products to our customers.
Code review are done in open discussions using pull requests:
- We learn by reviewing eachothers code.
- Changes to the codebase is reviewed and agreed before accepting it to
main. Modern change management in practice! - Triggers automatic tests of our code.
As we will be doing numerous code reviews in GitHub, we welcome anything that makes it faster, easier, and better. For this we have established a set of code review principles to help us reduce friction, make a review less of a burden and set correct expectations to leave us with more time to do awesome work.
When handling a PR, make sure you are treating your role as submitter and reviewer with the responsibility it requires! Together we, both submitters and reviewers, are responsible for what happends after our code is approved and merged.
If we break something, we fix it.
Small pull requests are win-win-win. They’re easier for reviewers to grasp right away and quicker to review since there’s less code to review and comment on. For submitters, this fast turnaround makes it more likely that changes get merged faster, and any team member who depends on the code can start working on their next project.
Respect eachother and don’t make it personal! We all make mistakes at some point. What’s important is to learn and improve, and to treat others how we’d like to be treated. Don’t criticize the person submitting the code, point out issues or technical problems. Conduct your review and feedback blamelessly. As a reviewer, you’re there to act as another set of eyes, provide a different perspective, and notice changes that cause unwanted side effects. The language and tone you use matters, so be mindful about how you phrase your comments. We WANT people to submit code, and review code. If we make the code review a hostile process, we won't get contributions.
Some might be biased, some might be outdated, and some of them are problematic in a particular context. It doesn’t matter. We work together, communicate openly, self-check our feedback, and try to be as objective and fact-based as possible. i.e. back your comments/feedback with links to applicable sources.
Engineers are problem solvers; that’s what makes our job fun, but don’t try to force your solution. Point out problems. Ask “how could we make this better?” not “here’s how we should fix X.” Ask clarifying questions rather than immediately concluding that the code is wrong and that you know how it should be. Communicate, collaborate, discuss and solve problems as a team.
After you have create the pull request, review it yourself first. Similar to proof-reading paper text, you’ll likely detect more errors. Many issues can be uncovered when we look at our own changes as a whole, and we’re able to fix them before anyone else spots them or wastes time catching a simple issue.
To avoid having PRs sit in limbo for days or weeks respond to PRs! If there is a PR tergeted to the team, check it and post your feedback. Check the code, do not just look at the description and trust that the code is flawless.
If you are specifically requested to provide feedback, be sure to do so as soon as time allows!
Not following this rule this results in:
- Bottlenecks: Long-running pull requests block people from continuing with their work, especially in situations where many others depend on the code being merged and deployed.
- Wasted work: As pull requests age, they become stale and out of sync with upstream branches unless the submitter regularly rebases or merges upstream changes while waiting for the initial review.
- Uncertainty: Open and unfinished pull requests add mental load for submitters; they need to constantly check status, make updates, and be on alert for reviewer comments or change requests.
Pull request comments easily become a rabbit hole, drifting off and focusing on a small, irrelevant area. Especially where there are differing opinions between submitters and reviewers, not objective facts, and results in a stalemate. Don't wait too long with parking such a discussion and instead raise them in the team meetings (standups/status meetings). Feedback should be actionable and concrete, not overly opinionated and theoretical.
This also means, as a submitter, you should make it faster for a reviewer to take a stand on your contribution. Describe parts of the code where you expect to get feedback.
Review your feedback immediately, ask any necessary questions, and make required changes. If your pull request sits, your reviewer’s feedback gets stale and it’s likely that other code conflicts will render it un-mergeable. Check for reviewer comments or requests frequently (every one or two days), and act quickly.
Use existing indentation style: Be consistent with what’s already there; nobody wants to have the age-old fight over tabs, spaces, 2 or 4, over and over again. Recognize and incorporate architectures and coding patterns: Maintainers need to establish and enforce some level of consistency in how things are structured or abstracted so projects don’t become a confusing mishmash. Identify these commonalities and adhere to them.
Clear contribution guidelines set contributor expectations from the get-go, simplify adding changes, and result in better code overall. If it’s easy to get started, you’re more likely to get more (and more frequent) contributions. This includes providing helpful advice for how to navigate the codebase and get started.
Its easier being reprimanded by a robot over having teammates call out mistakes like indentation style violations. Automatic linters are a code reviewer’s best friend; we simply run them automatically during the PR process to catch obvious flaws early. We use automation to catch easy mistakes and focus on giving more valuable feedback.
More advanced tools catch things that aren’t directly related to the code. For example, I’ve picked up that someone forgot to update a Changelog, if the pull request impacts critical pieces of the codebase, or vice versa when a pull request fixes documentation typos or other trivial changes.
This is a mock repo for testing