diff --git a/docs/programming/general/pull-requests.md b/docs/programming/general/pull-requests.md new file mode 100644 index 0000000..2d2e85b --- /dev/null +++ b/docs/programming/general/pull-requests.md @@ -0,0 +1,76 @@ +# Pull request practices + +Practices to make code review smoother and more productive. + +## Submit your final draft, not your first + +It may be tempting to get your work into code review as soon as possible, but if +you ask people to read your spaghetti code, then you'll find that your PR will +take longer to review, plus reviewers will burn their time and energy asking +you to fix obvious mistakes instead of checking for more subtle problems. + +Do not use code review as a linter. You should not let any mistakes get to code +review that you could have fixed by scrutinising your own work first. + +Review and polish your own PR before publishing it. + +- Have you refactored the code to make it readable and maintainable? +- Have you documented the code where needed? Does the PR description explain + all of the changes and answer questions the reviewers might have? +- Have you defended your implementation choices? +- Have you reviewed the diff to make sure there are no obvious mistakes? +- Have you looked at the diff in the code review UI and thought about how a + reviewer might need help to navigate this code? + +You should aim to be the harshest and most thorough reviewer of your own PRs. + +## Don't overcomplicate the diff + +While you're working on a ticket you'll often find other things that need to be +fixed in the codebase. Some refactoring, a bug, an idea to improve logging etc. + +Do not fix these things in your ticket branch. Because when you publish your PR, +you'll have to explain these extra changes to reviewers, or worse, leave them +guessing about why there are changes in the PR that don't seem to have anything +to do with the ticket. + +## Document against "huh?" + +The behaviour of your code, and the reasoning behind it, isn't always going to +be obvious to other devs reading the code, including you when you come back to +read your code in six months. + +You should always write readable, self-documenting code, but sometimes that +won't be enough, and you'll still need to explain _why_ the code is there in +the first place. + +For example, a line of code may solve a tricky corner case or do some other +operation that looks unnecessary at first glance. It's your job to recognise +when another dev will look at that code and say "huh?" Answer their questions +preemptively, by explaining what problem its solving and how. + +If reviewers still ask questions about your code, or make comments that suggest +that they didn't understand your code, then that's often a sign that you still +need to improve the code's readability and documentation. If a dev is confused +about your code now, then there's a good chance a different dev will be +confused about that code in future. + +## Give reviewers a primer + +Reviewing code should not be a challenge in puzzle-solving. You want the +reviewer to understand your changes quickly and easily, so they can decide if +your changes make sense. This helps them approve your code quickly, or suggest +improvements to your solution. Both outcomes are good for you! + +Reviewers see your PR changes as a list of diffs sorted by filepath. So if they +read the diff from top to bottom, they'll likely be jumping between different +aspects and levels of your solution, instead of starting at a high level and +working their way down to the details. Reviewers may need to go over your diff +several times before it makes sense. + +Use the PR description to explain, at a high level, what problems you solved in +the ticket and how you solved those problems. If you considered multiple +solutions, explain why you chose one over the other. + +Your effort on this should be roughly proportional to the complexity of the +diff and the problems you solved with those changes.