1# Reviewer
2
3A review is given by a reviewer on code contributions (see: [contributing.md](contributing.md)) of
4a reviewee. There are no guidelines on how review has to be performed in ILIAS, however here we propose a pattern
5inspired by an article of Dan Munckton in [How to be a kinder more effective code reviewer](https://cultivatehq.com/posts/how-to-be-a-kinder-more-effective-code-reviewer/).
6
7In our experience many conflicts arise due to feedback written in a fashion which makes it hard to understand,
8what exactly is asked of a reviewee. The pattern adapted from suggestions by Munckton is aimed
9to make reviews easier to read and to leave less room for misunderstandings and communicational deadlocks.
10
11
12
13## General Pattern
14We try to lay a strong focus on making feedback **actionable**, meaning the reviewee should have
15a clear picture on how to react. Inspired by Munckton (see below for more details) we propose
16to use the following structure to reply to PRs:
17
18```
19Hi @[name_of_reviewee]
20
21Thank you a lot for contributing to ILIAS.
22
23[reaction_not_actionable]
24
25Please answer the following questions:
26- [ ] [question1]? [optional: reasoning behind the question]
27- [ ] [question2]? [optional: reasoning behind the question]
28- ...
29
30Please consider the following suggestions. You do not need to follow those, but but please indicate shortly why you prefer to do otherwise:
31- [ ] [suggestion1] [optional: reasoning behind the suggestion]
32- [ ] [suggestion2] [optional: reasoning behind the suggestion]
33- ...
34
35Please implement the following changes:
36- [ ] [change_request_1] [optional: reasoning behind the suggestion]
37- [ ] [change_request_2] [optional: reasoning behind the suggestion]
38- ...
39
40[optional_comments_on_how_to_proceed]
41
42kindly,
43@[your_username]
44
45```
46This might look as follows:
47
48```
49Hi @klees
50
51Thank you a lot for contributing to ILIAS.
52
53We highly apreciate the effort you took to improve the JS in the KS component. I especially
54like the new proposed pattern to tackle the scoping issue.
55
56Please answer the following questions:
57- [ ] Why did you not use ECMAScript 6? Note that Bootstrap 4 makes use of it.
58
59Please consider the following suggestions. You do not need to follow those, but but please indicate shortly
60why you prefer to do otherwise:
61- [ ] Change XY to pattern XZ. We believe that pattern XZ might be superior. However, I am not completely sure.
62
63
64Please implement the following changes:
65- [ ] Reword the function addProperty to withProperty. withProperty is in line with our naming scheme used
66  in othe methods of the framework.
67
68
69Please give a feedback until the end of the week indicating how long it will take to answer
70the questions given, react to the suggestions and implement our change requests.
71
72
73kindly,
74@amstutz
75
76```
77
78In short, we have the following suggestions for you when writting reviews:
79- [ ] Use the above template. Reasoning: See next chapter.
80
81## Reasoning
82Munckton introduces the term **actionable** for feedbacks. An **actionable** feedback is one that
83can directly be reacted upon. He claims Questions, Suggestions and Change Requests
84to be the most relevant types of such statements. He gives the following examples of those three statements:
85
86- Question: could you explain why this ended up having to be here?
87- Suggestion: you may have come across this already, but we might be able to use XYZ for this.
88- Change request: sorry, I appreciate the effort it took to get it this far. But the team
89already agreed to solve this using XYZ. So I'm going to have to ask you to rework this. Let me know if I can help.
90
91
92A fourth type he sometimes uses is the reaction, which is not actionable, but might help to express emotions such as thanks.
93
94He further suggests to always keep things as clear as possible by using the following pattern:
95
96>[type]: [actionable request]
97
98>[rationale or discussion]
99
100Whereas:
101- type: one of Question, Suggestion or Changes Request.
102- actionable request: is a short clear question or statement, which makes clear how the reviewee
103should respond.
104- rational or discussion: Possible further explanations of the reasoning behind the request.
105
106An example could be:
107>Suggestion: use XYZ.
108
109>You may have come across this already, but we might be able to use XYZ for this because blah blah blah etc.
110
111We adapted this slightly by bundling questions, suggestions and change requests in grouped listings and placing
112the reactions up front.
113
114## Inline Comments
115Github makes it very easy to leave inline comments. However, in the past, we observed, that large
116numbers of inline comments make a review hard to read and understand. We therefore propose
117only the leave inline comments for trivial directly actionable suggestions and change request, that
118do not need an rational (E.g. *Change Request: typo in word actionalbe*).
119
120Do not leave any reactions in inline comments. They tend to crowd the discussion without giving
121the possibility of being resolved (seens not actionable). Move them to the summary.
122
123
124
125
126