1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
|
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=utf-8">
<link href="wayland.css" rel="stylesheet" type="text/css">
<title>Wayland - Reviewing Patches</title>
</head>
<body>
<h1><a href="/"><img src="wayland.png" alt="Wayland logo"></a></h1>
<h1>Reviewing Patches</h1>
<p>
TL;DR: if you want to help, the best thing you can do right now is review
other people's patches on the mailing list.
</p>
<p>
Longer:
</p>
<p>
Everyone can help in reviewing, from the newest member of the community to the
oldest. Reviewing is basically two separate operations:
</p>
<p>
(1) Technical (objective) review:
</p>
<ul>
<li>Is the code correct?</li>
<li>Does it compile? Does it introduce new bugs?</li>
<li>Does it fix the problem that it proposed to fix? Or does it successfully implement the feature it proposed to implement?</li>
<li>Will it pose future problems, architecturally, security-wise, etc.?</li>
<li>Does it have unit tests? Is it documenting the API? </li>
<li>Is the code style correct?</li>
<li>...</li>
</ul>
<p>
(2) Subjective review:
</p>
<ul>
<li>Does this solution belong in Wayland or Weston?</li>
<li>Is it in scope for the project?</li>
<li>Is it the right solution for the problem?</li>
<li>Is the API easy to use? Not confusing?</li>
<li>Is the code readable, and properly commented?</li>
<li>Is this the right time for this? Is this the best use of resources?</li>
<li>...</li>
</ul>
<p>
In both cases, the more experience you have in reviewing, the better the
review will be and the easier it will be to review. However, for a new
contributor, starting with the technical is easier, since it's objective. In
time, you'll learn how to do the subjective review too.
</p>
<p>
Also, don't be afraid of doing bad reviews. Other people will review the same
contribution and review reviews. If a mistake is found, everyone learns. For
the subjective review, there is really no right or wrong, but a consensus of
the community. So speak up.
</p>
<p>
Especially, speak up if the patch looks good. If you think it passes the
review criteria, <i>say so</i>. Reply to the email saying "This looks good", "Ship
it!" or "Awesome, thanks!" (praise goes a long way towards making people
feel welcome). In addition, add the line <em>Reviewed-by: Real Name
<my@email.address></em> so people pushing patches upstream can make it
<a title="example reviewed commit message"
href="https://cgit.freedesktop.org/wayland/weston/commit/?id=273874e3c715a7102add0030f200d0a0f17628fa">part of the commit message</a>.
</p>
<p>
Kristian and other designated people with repository access will gladly accept
those reviews and apply the contribution to the codebase. Of course, they
reserve the right to re-review and point out if there were any problems. But
just like above, if that happens, everyone learns.
</p>
<h2>Why you should do this?</h2>
<p>
Well, if you're here, it's because you want Wayland and/or Weston to succeed.
If you contribute a bit of your time and expertise, you're helping the project
achieve that goal.
</p>
<p>
If you have patches you have submitted yourself, by reviewing you free up the
time from other reviewers, increasing the chances that your contribution will
be dealt with sooner. Think also of this as tit-for-tat: if you want code
reviewed, review code too.
</p>
<p>
You'll also gain experience in reviewing. You'll become more familiar with the
codebase. That might come in very handy if you're doing this as part of some
company project.
<p>
And you'll gain merit in the project. The more you contribute (in reviews or
in code, but those go hand-in-hand), the more you'll be respected and the more
your own opinions will be listened to. This is important when it comes to
consensus-building and decision-making: Open Source Projects are not
democracies and definitely not tyrannies. They are meritocracies, where those
who have contributed the most get to make decisions.
</p>
</body>
</html>
|
