77 lines
3.6 KiB
Markdown
77 lines
3.6 KiB
Markdown
|
# 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.
|