You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Copy file name to clipboardExpand all lines: guides/CODE_REVIEW.md
+22-4Lines changed: 22 additions & 4 deletions
Original file line number
Diff line number
Diff line change
@@ -5,17 +5,21 @@ There are **\*three types of Code Review** inspired by conference talks by Trish
5
5
-https://www.youtube.com/watch?v=a9_0UUUNt-Y
6
6
-https://www.youtube.com/watch?v=3pth05Rgr8U&t
7
7
8
-
## Draft / Early Feedback
8
+
## The Three Types
9
+
10
+
### Draft / Early Feedback
9
11
10
12
Github has a feature to mark a Pull Request as a draft. Often this won't run CI checks as it is known that it is in a partial working state.
11
13
12
14
These are great to get early feedback about design decisions before sinking too much effort.
13
15
14
-
## Educational
16
+
### Educational
17
+
18
+
Sometimes the person creating the Pull Request is the Subject Matter Expect (SME).
15
19
16
-
Sometimes the person creating the Pull Request is the Subject Matter Expect (SME) like a @newwwie/core-maintainers. They have authority to just push the change through, but it is important for others to see and follow along what the change _**IS**_ and _**WHY**_.
20
+
They have authority to just push the change through, but it is important for others to see and follow along what the change _**IS**_ and _**WHY**_.
17
21
18
-
## Goalkeeper
22
+
###Goalkeeper
19
23
20
24
This is probably the more common one that people think of when it comes to code review. They are looking at quality control and trying to stop bugs getting through.
21
25
@@ -29,3 +33,17 @@ Sometimes a solution isn't exactly how you would expect it to be solved, that is
29
33
**Nitpicking should never block code getting released**.
30
34
31
35
The regulating factor here is that if the code could not reasonably be maintained by someone else, then that is leaving tech debt and lowering the bar for the whole project.
36
+
37
+
## Before Code Review
38
+
39
+
Before requiring the valuable resource which is someone's attention there are some things you can do, (and yes your attention too is valuable, and I am grateful you have read this far 🫶).
40
+
41
+
- Run code formatters
42
+
- Run code linters
43
+
- Run test suite
44
+
- Add test coverage
45
+
- Read your own PR as though someone gave it to you for review
46
+
- Does the code represent the pragmatically smallest amount of code changed to reach the outcome?
47
+
- Can you squash some of the commits together to better tell a story? Eg 17 commits down to 3 commits?
48
+
- Can you pre-emptively add comments on PRs to draw attention to the important parts?
49
+
- Have you added appropriate documentation so the next person can self-service contribute?
0 commit comments