Code review & response

aka «The Hard Part»

Колега написала код, опублікувала та описала його. Тобі прийшло повідомлення, що вона призначила code review на тебе. Що робити далі?

Серія записів про code review:

1. Навіщо?
2. Створення pull request’у
3. Думки про pull request assignee
4. Code review & response

Англійською: On Code Reviews

Слухати

У кожної зміни в pull request’і є ціль. Окрім очевидних зовнішніх «виправити баг», «додати фічу»¹ і «оновити код/залежності», ціль може бути внутрішньою, наприклад:

• бути проміжним кроком у виправлені багу/додаванні фічі («ця функція експортується, тому що вона використовується в неопублікованому коді»)
• вирівняти існуючий код з ментальною моделлю у голові авторки pull request’у («воно було незрозуміло, я переписала, щоб стало зрозуміліше»)
• підвищити зручність розробки («я вмію робити grep, а цей файл цьому чинив сильний опір»)

Ревʼювер може не погоджуватися з цінністю кожної окремої цілі або шляху її досягнення, але для гарного code review треба їх знати і, за необхідністю, вирівнювати їх з цілями решти проєкту

Тому перед тим, як шаленіти через "говнокоду" (що б це не значило), задумайся над ціллю, яку переслідувала авторка. З зовнішніми цілями тобі допоможуть текст задачі або фонові обговорення. Внутрішні ж найчастіше можна зрозуміти хіба що за контекстом в історії комітів або в описі pull request’у

Якщо ж контексту авторка залишила небагато, то можна допомогти собі фільтрацією змінених файлів, переглядом її минулих змін, локальним підняттям гілки pull request’у, спробою переписати «дивне рішення» з урахуванням «ну, це ж так не робиться»…

Ну, або у краааааайньому випадку, можна запитати її напряму

Говорити

Кожен коментар ревʼювера, так само як і кожна зміна у pull request’і, служить для чогось. Наприклад, за допомогою коментарів, ревʼювер може…

Дізнаватися

«Жанр» коментарів, який є найбільш наближеним до головного призначення code review, але який рідко використовують, це «Чому написано саме так?». Навіть коли усі учасники дискусії знають причину, але вона ніде не згадана явно, необхідність сформулювати відповідь допоможе помітити надмірну складність

Якщо вони думають, що знають причину, то явне формулювання точки зору однієї з учасниць code review може виявити випадки, коли ці знання є лише припущеннями. А припущення ведуть до багів, тож іноді краще уточнити щось «очевидне». Якщо ж це «очевидне» вже було десь розписаним, то нічого жахливого у тому, щоб отримати посилання на коментар² з відповіддю або на коміт з поясненням

На жаль, цей жанр не лише найважливіший (на мою думку), але й найнебезпечніший. Прочитавши «Чому написано саме так?» у поганому настрої, я можу накрутити себе думками «та як це вони сумніваються у моїй праці?!» та/або «схоже, тут так неможна, тому, замість відповіді на питання, я усе зараз же перепишу». Тому дуже важливо дати авторці pull request’у зрозуміти, що у твоєму питанні немає ніякого прихованого сенсу. У тексті інтонація передається надзвичайно погано. Тож, щоб текстом коментар читався настільки ж позитивно, як якби його було почуто вголос, цю недостачу інтонації треба компенсувати. Наприклад, перефразувавши питання, виділивши наголос курсивом, явно написавши «пробач, я тут ні про що не натякаю», або додавши краплю емоцій емоджами

Коригувати

Набагато популярніший жанр коментарів до pull request’ів — це «тут неправильно». Багато хто вважає, що ці коментарі — найважливіше у code review. Так, додаткова пара очей допоможе виявити проблеми раніше, але, повертаючись до початку цього запису, виключно важливо розуміти цілі змін перед тим, як їх критикувати. Без цього розуміння, обидві сторони code review будуть псувати настрій одна одній — або шкідливими порадами, або поверхневими правками для «відстань»

І навіть якщо ти знаєш, яку ціль переслідувала авторка в цій зміні, то…³

Зовсім необовʼязково просити виправити кожен рядок, в якому бачив проблему. По-перше, проблеми може не бути, тоді «чому?» спрацює краще. По-друге, особистий досвід запамʼятовується краще чужого, тож якщо проблема некритична та/або швидко проявиться та/або легко сховається від користувачів, то десять хвилин паніки навчать багато чому. Наприклад, сім років тому, я так навчився що нові фічі треба ховати за feature toggle і що кеш треба інвалідувати

Коли ж проблема дійсно варта коментаря, то усі схожі коментарі краще залишити за раз, щоб не затягувати час до закриття pull request’у очікуванням фіксів/коментарій. Іншими словами, «коментар-коментар-фікс-фікс» краще за «коментар-фікс-коментар-фікс»

Окрім вказівки на проблему, ревʼювер може запропонувати її вирішення. Github дозволяє зручно це зробити прямо у веб-морді

Якщо маєш гарні відносини з авторкою pull request’у і ви не сумніваєтеся у здібностях одна одної, то можна склонувати собі гілку і запропонувати покращення у вигляді комітів. Куди пушати ці покращення — у нову гілку (з відкриттям pull request’у до pull request’у⁴) або в ту ж саму — залежить від того, як до цього відноситься авторка оригінальної гілки. Це, як і «дозволити помилитися», доволі ризикований шлях, але він існує

Повертаючись до втрати інтонацій в тексті… Якщо людина може неправильно зрозуміти невинне «чому?», то «я написав код для твого pull request’у» або яскраво червоне «@username requested changes» точно рано чи пізно будуть прийняті особисто. Тож їх треба навмисно помʼякшувати або хоча б нагадувати усім, що це критика коду, а не людини

Але навіть «@username requested changes» краще тиши

Форматувати

Що не треба коментувати, так це крапки-з-комою, відступи та інше форматування. Якщо форматування коду дійсно важливе, то замість написання коментаря «ми ставимо пробіли всередині фігурних дужок», витрати свої зусилля на просте автоматичне виправлення форматування⁵ і на запуск лінтеру в CI-середовищі⁶

Без цього, ревʼюверу треба постаратися, щоб утримати фокус на сенсі pull request’у і не перемикатися на коментування поверхневих та часто марних для кінцевих користувачів «відсортуй імпорти за абеткою»

Мати на увазі

Коментарі ревʼюверів можуть бути адресовані не виключно авторці pull request’у, але і іншим колегам

Наприклад, щоб звернути увагу до складності інтеграції з API іншої частини системи. Або до більшої-ніж-хотілося-б кількості мануальних змін там, де рефакторінг/типи/автоматизація спростили б та скоротили б майбутні pull request’и. Або до схожості нового коду на код у іншому проєкті і до можливого code share

Одразу реалізувати ці коментарі, у тому ж pull request’і, скоріш за все, зайве збільшить його scope, але, через пару таких коментарів-нагадувань, буде легше згадати про них підчас планування наступного спрінта

Заохочувати

Якщо після відкриття pull request’ів отримувати лише описані раніше нейтральні та відносно негативні коментарі, то звичайно люди будуть уникати code review — «Я намагалася, працювала, оминала проблеми, тільки щоб перед самим фінішем слухати, що тут дивно, тут неправильно, тут погано…»

Тому, у противагу всьому цьому, корисно хоча б іноді хвалити особливо хороші зміни, будь то витончене вирішення проблеми, гарна ідея (яку ревʼювер може застосувати у своєму коді), або банальне «дякую, давно болить, але руки ніяк не доходили»

Без коментарів, code review — це лише вахтерство. З коментарями лише з серії «тут неправильно, виправ» — шкидливий gatekeeping. Сподіваюсь, цей запис продемонстрував, що з правильними коментарями, pull requestʼи стають набагато корисніше всім його учасникам

Дякую Богдану Бузу та Олександру Вєрінову за допомогу у написанні цього запису