Понедельник, 10:37. Андрей открывает пул-реквест: рефакторинг платежей, 847 строк, три дня. В 11:42 Дима ставит LGTM. Тридцать секунд. В 12:15 я ставлю Approved, пролистав файлы и зелёные тесты. Две минуты. В 14:00 вливают в мастер. В 14:47 прод не принимает деньги.
Раньше система трижды повторяла запрос к провайдеру. После рефакторинга отменяла транзакцию сразу. Четыре часа, около 40% оплат мимо, 127 000 упущенной выручки, шесть часов хотфикса, 230 тикетов в поддержку.
Два опытных человека пропустили дыру, потому что ревью было театром. Галочка перед мержем. Это было четыре года назад. С тех пор я больше не считаю скорость первого аппрува победой.
Как понять, что процесс уже спектакль
Ревью меньше минуты на две сотни строк, комментариев нет или они про пробелы. Война из-за camelCase, архитектура молчит. Пул-реквест висит три дня, автор ушёл на другую задачу, контекст умер. Фразы «это ужасно» и «зачем ты так». Никто не знает, сколько багов ловит ревью и сколько пул-реквестов старше суток.
Человек думает: надо пройти ревью, чтобы влить. Должен думать: надо услышать, пока ещё дешево. Критериев нет, каждый смотрит своё. В дашборде живут velocity и закрытые тикеты, ревью не живёт нигде. Автор боится критики и режет эксперимент. Ревьюер боится обидеть и ставит LGTM.
Что смотреть, и в каком порядке
Сначала машина. Тесты, линтер, формат, сканер секретов и зависимостей, покрытие не упало без объяснения. Если человек на ревью спорит про отступы или гоняет тесты руками, процесс сломан. Красный CI ревью не начинает.
Потом поведение, 5-10 минут. Тикет закрыт? Что будет на null, пустом списке, нуле, огромном числе? Где упадёт без обработки? Скидка для VIP без проверки пользователя и суммы в пятницу уже лежит на проде.
function calculateDiscount(user, total) {
if (!user || total <= 0) throw new Error("Invalid input");
return total * (user.isVIP ? 0.2 : 0.1);
}Потом посадка в систему. Нет ли второго такого же валидатора почты в соседнем методе. Не повёрнута ли зависимость. Не родился ли новый бог-объект. Сюда же N+1: тысяча пользователей и запрос заказов в цикле это тысяча один поход в базу.
# Так падает касса на отчёте
for user in User.objects.all():
orders = Order.objects.filter(user_id=user.id).count()
# Так ходит один раз
User.objects.annotate(orders_count=Count("orders"))Потом безопасность, коротко. Строка запроса с email внутри, ключ в коде, проверка «вошёл» вместо «имеет право», дырявый ввод на внешнем API.
# email = "' OR '1'='1" → вся таблица
query = f"SELECT * FROM users WHERE email = '{email}'"
query = "SELECT * FROM users WHERE email = ?"
db.execute(query, [email])Потом чтение через полгода. function p(u, a) с магическими 1 и 0.15 в проде жить не должна. Имя, константа, короткая функция. Комментарий там, где ход неочевиден, не там, где код и так говорит.
Потом тесты. Счастливый путь, край, ошибка. Один assert на VIP и сотню рублей дыру не держит. Нужны ноль, пустой пользователь, не-VIP. Иначе рефакторинг снова отменит ретрай, а тесты будут зелёные.
Автор перед запросом сам читает дифф как чужой, пишет зачем этот ход, ставит связанный тикет, помечает ломающие изменения. Ревьюеру в шаблоне те же полки: поведение, край, посадка, скорость, дыра в безопасности, чтение, тесты. Вопрос внизу, если сам не уверен.
## Что меняет этот PR
## Тикет
## Тип: баг / фича / ломает / рефакторинг
## Автор: сам прочитал, тесты гонял, секретов нет
## Смотрящему: поведение, край, посадка, скорость, дыра, чтение
## Где я сам не уверенЧеловека в комментарии нет. «Ты написал ужасный код» закрывает следующий эксперимент. «Этот ретрай без паузы положит API, вот экспоненциальная задержка, как смотришь?» оставляет работу.
Префиксы экономят кровь.
[CRITICAL] SQL через f-string в строке 47. Параметры, не склейка.
[MAJOR] N+1 на 10 тысяч строк. Нужен select_related.
[MINOR] data слишком общее имя.
[NITPICK] Мне ближе guard clause, на вкус.
[QUESTION] Почему setTimeout, а не промис?
[PRAISE] Мемоизация здесь к месту, сам бы не догадался.
Критичное блокирует мерж. Придирка не блокирует. Похвала тоже работа: иначе в ленте одни дыры.
На фидбек не надо отвечать в ту же минуту обороной. Если критика верная, чини. Если спорная, напиши, почему оставил. Если не понял, спроси. «Переделывай» без примера можно вернуть просьбой показать ход.
Формат и линт живут в pre-commit и в CI, не в голове сеньора. Набор инструментов вторичен. Важно, чтобы красное не доезжало до человека.
Хуки у нас гоняют линт, формат и быстрые тесты на том, что в индексе. CI на пул-реквесте: те же проверки плюс покрытие и сканер зависимостей. Если красное, смотреть дифф рано. Иначе снова 30 секунд на 847 строк, только уже с зелёной галочкой линтера и живой дырой в ретрае.
Автор имеет право не соглашаться. Верная критика чинится. Спорная остаётся с короткой причиной в треде. Непонятная возвращается вопросом. «Переделывай» без примера можно отослать назад: покажи ход. Так Павел перестал воевать, когда ему запретили оценку человека и оставили оценку кода с префиксом.
Что считать, чтобы театр было видно
Время до первого взгляда: для пул-реквеста до 200 строк хочу часы, не дни. Для 847 строк аппрув за 30 секунд запрещён самой физикой чтения. После случая с платежами поставили пол: больше 200 строк нельзя аппрувить раньше чем через 30 минут.
Доля диффа с живым комментарием, не «LGTM». Баги, пойманные здесь, против багов на проде. Сколько пул-реквестов старше суток. Размер: стопка по 800 строк почти всегда читается по диагонали. Лучше резать.
Команда из восьми до того случая: первый взгляд за 15 минут (казалось, победа), комментарии на 12% диффа, 8 продовых багов в месяц. После чек-листа, пола по времени, цели больше 80% покрытия комментариями и правила «двое смотрящих, хотя бы три замечания на сотню строк» через квартал: первый взгляд 4 часа, покрытие 78%, продовых багов 2, на ревью ловят 12 в месяц. Медленнее до мержа. Дешевле, чем четыре часа мёртвой кассы.
Второй случай того же года. Сеньор Павел писал Андрею: ужасный код, переделывай, как ты думал, что это будет работать. Андрей ушёл в минимальные диффы, через два месяца уволился. Разговор с Павлом, префиксы, запрет на оценку человека. Через два месяца новый джун назвал Павла лучшим ментором. Тон меняется дольше, чем экшен в CI. Без тона чек-лист становится новым театром.
Третий: пул-реквест пять дней без взгляда. Автор уже в другой задаче, возвращается в свой же код как в чужой. Правило: если сутки никто не взял, пинг в канал, слот в календаре как у встречи. Ревью это работа, её планируют, не выхватывают из воздуха в пятницу в шесть.
Внедрение у меня занимает не квартал курсов. Неделя: договориться, что LGTM без чтения больше не живёт, повесить шаблон и префиксы. Неделя: линт, тесты и секреты в CI, красное не ревьюят. Две недели: пол по времени на большие диффы, ротация, чтобы одни и те же два человека не ставили галочки друг другу. Дальше смотреть продовые баги и возраст пул-реквестов раз в две недели и резать то, что снова стало театром.
Третий случай того же года я уже коротко назвал: пять дней без взгляда. Добавлю механику, которой потом закрыли. Если сутки никто не взял, бот в канале пишет имя дежурного по ротации. Слот 45 минут стоит в календаре как встреча. Пул-реквест больше 400 строк просим разрезать, иначе читают по диагонали, как те 847. Автор имеет право пингануть через сутки без ощущения, что клянчит.
Человек на ревью не экзаменатор и не нотариус. Он второй, кто может поймать отмену платежа до того, как её поймают 230 клиентов. Если после его взгляда все знают, что делать с кодом, слот окупился. Если после его взгляда стоит только большой палец, слот уже выставил счёт. Мы этот счёт однажды оплатили.
