Ревью на ясность и тон
Что это такое
Вторая половина ревью — это коммуникация: сказать о найденном так, чтобы автор мог это исправить, и решить, какие находки достаточно важны, чтобы задержать вливание. Ревью — письменный артефакт о чужой работе, сделанный в спешке, и его тон определяет, поймёт ли автор суть или начнёт защищаться. Ясность и тон нужны не из вежливости ради неё самой; они делают ревью действенным.
Почему это важно
Ревью, перечисляющее двадцать равнозначных комментариев, ничему не учит автора о приоритетах и замедляет вливание до черепашьего шага. Ревью, которое читается как личная оценка, делает авторов защищающимися: тот, кто защищается, объясняет, а не исправляет. Обе неудачи распространены и обе чинятся несколькими привычками: отделять блокеры от предложений, быть конкретным насчёт кода, а не человека, и держать дифф достаточно малым, чтобы внимательное ревью вообще было возможно.
Ключевые идеи
Отделяйте блокеры, предложения и придирки
Скажите, что есть что, словами. Блокер — то, что должно измениться до вливания: дефект, проблема безопасности, нарушенный контракт. Предложение — «я сделал бы так, подумайте». Придирка — мелочь, и назвать её таковой ничего не стоит: так автор не примет её за приказ. Без меток читатель приписывает наивысшую срочность всему.
блокер: эта запись не в одной транзакции с чтением выше
предложение: объект-значение для start/end сделал бы проверку диапазона очевидной
придирка: опечатка в docstringБудьте конкретны и говорите о коде
«Запутанно» не выполнимо; «я не понял, включает ли total налог — имя намекает, что нет» — выполнимо. Укажите на строку, опишите, что ожидали и что видите, и оставьте человека в стороне. Это ещё и самый быстрый путь достойно ошибиться: конкретное утверждение можно опровергнуть доказательством, и разговор движется вперёд.
нет: "плохое имя"
да: "`data` хранит разобранную запись, а не сырой ответ — переименовать?"Малые диффы ревьюят лучше
Большой дифф получает более поверхностное ревью, чем малый, и каждый ревьюер это знает. Разбиение изменения на последовательность малых, по отдельности понятных кусков поднимает качество внимания к нему и делает полезным более поздний bisect. Прося о ревью, предпочитайте несколько малых изменений одному большому и скажите, в каком порядке их читать.
хорошо: рефакторинг (без смены поведения) -> затем смена поведения
плохо: один дифф, делающий и то и другое, и приписка, где какие строкиАвтор управляет качеством ревью
Опишите замысел, укажите части, в которых не уверены, и проведите самопроверку диффа, прежде чем просить. Изменение, поданное с «не уверен, что цикл повторов верен», получает внимание ровно там, а брошенное без контекста — ревью на стиль. Ревьюер не читает мысли, и просьба присмотреться к конкретному решению — не признание слабости.
Принимайте ревью, не принимая на свой счёт
Исходите из того, что ревьюер внимательно прочитал код и хочет, чтобы изменение удалось. Отвечайте по существу, просите уточнений там, где комментарий неясен, и говорите, когда не согласны и почему. Если комментарий — вопрос вкуса, законно сказать это и идти дальше, но проверьте, не поднималась ли та же мысль дважды, прежде чем объявлять её вкусом.
Практика
Разметьте своё следующее ревью
В следующем ревью поставьте блокер:, предложение: или придирка: в начало каждого комментария и сделайте один проход, который удалит всё, что не изменило бы следующий час автора.
Готово, когда: автор может перечислить ваши блокеры одной строкой, не перечитывая ветку, и вы убрали хотя бы один комментарий как шум.
Разделите изменение перед просьбой о ревью
Возьмите следующее изменение и разделите его на структурный коммит и поведенческий. Просите о ревью структурного первым.
Готово, когда: структурный дифф не содержит смены поведения, и вы можете сказать, насколько дольше из-за этого шло ревью.
Частые ошибки
- Перечисление каждого наблюдения как равного — автор не может понять, что обязано измениться; исправить: помечайте блокер, предложение и придирку.
- Комментарий о человеке, а не о строке — обсуждение становится защитным, а дефект остаётся неисправленным; исправить: назовите место, ожидание и наблюдение.
- Просьба о ревью огромного диффа — ревью превращается в беглый просмотр, и настоящие дефекты проскакивают; исправить: делите по темам и указывайте порядок чтения.
- Молчаливая перезапись ветки автора — автор ничему не учится и теряет авторство; исправить: комментируйте или работайте над изменением вдвоём.
- Каждое предпочтение как требование — кодовая база впитывает вкус ревьюера, а не стандарт; исправить: оставьте блокеры для дефектов и согласованных договорённостей.
Что читать дальше
- Чтение диффа на корректность (01-cs-fundamentals/17-code-review/01-reading-a-diff-for-correctness) — на что смотреть до того, как решать, что сказать, поэтому прочтите первым.
- Чистый код (01-cs-fundamentals/08-clean-code) — общий стандарт, при котором комментарий о стиле становится ненужным.
- Рефакторинг (01-cs-fundamentals/19-refactoring) — как держать структурные и поведенческие изменения порознь, что и делает дифф ревьюируемым.