Added pull request notes
This commit is contained in:
parent
4438d0fbe8
commit
0abbe4d551
1 changed files with 76 additions and 0 deletions
76
docs/programming/general/pull-requests.md
Normal file
76
docs/programming/general/pull-requests.md
Normal file
|
|
@ -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.
|
||||
Loading…
Add table
Add a link
Reference in a new issue