Глава 7. Pull requests и code review

Введение

Pull request — самый частый жанр письменного английского в жизни разработчика. Писем ты можешь не писать месяцами. А PR открываешь каждую неделю и каждый день оставляешь комментарии в чужих — между делом, обычно не перечитывая. Поэтому язык здесь инструмент, а не украшение: от описания зависит, сколько ревьюер провозится, прежде чем поймёт изменение, а от формулировки замечания — закроют его коммитом за десять минут или будут обсуждать три дня.

Про три дня — не фигура речи. Я как-то оставил под чужой функцией комментарий This is wrong. и ушёл спать. Утром там висели три абзаца объяснений, почему автор сделал именно так; к вечеру подключился его тимлид; на третий день мы созвонились и выяснили, что прав я был ровно в одной строчке из двадцати. Напиши я тогда I think this misses the empty-list case — всё закрылось бы одним коммитом до обеда.

У русскоязычного разработчика с PR обычно две беды, и обе не про грамматику. Первая — описание в стиле fix bug: код же видно, чего расписывать. Вторая — та самая прямота. «Это неправильно, перепиши» по-русски звучит как техническая констатация, а This is wrong. Rewrite it. по-английски — как выговор от начальника подчинённому. Сначала разберём, как упаковывать своё изменение, потом — как разговаривать о чужом.

Язык ревью — не набор реверансов ради красоты. Это протокол, который держит цену недоразумения в разумных пределах, когда люди никогда не виделись лично, живут в шести часовых поясах и не слышат интонацию. Всё, что в разговоре сделал бы голос — пауза, усмешка, вопросительная интонация в конце, — в PR приходится делать словами.

Заголовок pull request

Заголовок PR увидят все, даже те, кто не откроет диф: он попадёт в список открытых PR, в уведомление в Slack, в письмо на почту, а после squash-merge — ещё и в историю коммитов навсегда. Работа у него ровно одна: за одну строку сказать, что меняется.

Делается это глаголом в повелительном наклонении (imperative mood) — будто ты отдаёшь команду кодовой базе. Соглашение пришло из гайдлайнов Git: заголовок должен логично продолжать фразу If applied, this commit will… («если применить, этот коммит…»). Подставь её мысленно перед своим заголовком — сразу слышно, годится он или нет.

Bad:  fixes
Bad:  Update UserService.java
Bad:  Fixed the bug where the cart was not updating sometimes when user

Good: Fix cart total not updating after coupon removal
Good: Add retry with exponential backoff to payment webhook
Good: Remove unused legacy image resizer

fixes не говорит ничего. Update UserService.java повторяет то, что и так написано в дифе: файл назван, смысл — нет. Третий вариант расползся и оборвался на полуслове. Хорошие все устроены одинаково: глагол (Fix, Add, Remove), объект и, если без него непонятно, условие — «after coupon removal».

Многие команды используют conventional commits — формат type(scope): description. Типы стандартные: feat (новая функциональность), fix (исправление), refactor (изменение кода без изменения поведения), docs, test, chore (рутина: зависимости, конфиги), perf (производительность). Выглядит как бюрократия, но у неё есть потребитель: по этим префиксам робот собирает CHANGELOG и считает следующую версию по semver. Назвал фикс словом feat — получил минорный релиз вместо патча.

feat(auth): add password reset via email
fix(cart): recalculate total after coupon removal
perf(search): cache facet counts for 5 minutes
chore(deps): bump django from 5.2 to 6.0

Две грамматические детали, на которых спотыкаются почти все. Глагол — в базовой форме: add, не adds и не added. Описание — с маленькой буквы и без точки в конце; это соглашение, а не безграмотность, и поправлять его в чужом PR не надо. Если изменение ломает обратную совместимость, ставят восклицательный знак и объясняют в теле: feat(api)!: drop support for v1 tokens.

Описание: What / Why / How / How to test

Тело PR существует ради того, чего в дифе нет. Диф честно показывает построчно, что изменилось, и ни слова не говорит, зачем и какие три варианта ты перебрал, прежде чем остановиться на этом. Стандартный набор секций:

• What — что сделано, в двух-трёх предложениях человеческим языком.
• Why — зачем: какая проблема, какой тикет, какая жалоба от пользователя.
• How — как именно решено, если решение неочевидное; здесь же отброшенные альтернативы.
• How to test — по шагам, чтобы ревьюер мог проверить руками.
• Screenshots — для любых визуальных изменений, желательно «до/после».
• Risks — что может сломаться, нужна ли миграция, нужен ли фича-флаг, как откатывать.

Многие репозитории кладут заготовку в файл .github/PULL_REQUEST_TEMPLATE.md, и GitHub подставляет её в каждый новый PR. Типичный шаблон:

## What
<!-- One or two sentences. What does this PR change? -->

## Why
<!-- Link the issue. What problem does it solve? -->
Closes #142

## How
<!-- Only if the approach is not obvious. Mention alternatives you rejected. -->

## How to test
1.
2.
3.

## Screenshots
<!-- Before / after for any UI change. -->

## Risks
- [ ] Requires a database migration
- [ ] Behind a feature flag
- [ ] Needs a config change in production

Closes #142 (синонимы — Fixes, Resolves) не просто ссылка: GitHub закроет issue при мердже сам. Если PR относится к задаче, но не закрывает её, пиши Related to #142 — иначе тикет схлопнется раньше времени, и кто-нибудь потом полдня будет искать, куда делась половина работы.

Теперь заполненный пример. Мерять его стоит не длиной, а числом вопросов, которые ревьюер после него НЕ задаст.

## What
Adds a retry with exponential backoff to the payment webhook handler.
Failed deliveries are now retried up to 5 times over roughly 15 minutes.

## Why
Closes #517. The provider occasionally returns 503 during their
maintenance window. We currently drop those events, so around 40 orders
per week stay in `pending` and have to be fixed by hand.

## How
The retry lives in the handler rather than in the queue config, because
we only want to retry 5xx responses — a 400 means the payload is wrong
and retrying it forever would just fill the queue. I considered using
the built-in queue retry, but it cannot distinguish between the two.

## How to test
1. Run the app with `make dev`.
2. Point `PAYMENT_WEBHOOK_URL` at the mock in `tests/fixtures/flaky.py`.
3. Trigger a payment. The mock fails twice, then succeeds.
4. Check the logs: you should see two `retrying webhook` lines.

## Risks
- Adds a new setting `PAYMENT_WEBHOOK_MAX_RETRIES` (default 5).
- No migration, no data change. Safe to revert.

Стиль: короткие предложения, настоящее время (Failed deliveries are now retried, а не will be retried), числа вместо оценок — «около 40 заказов в неделю», а не «часто». Секция How объясняет не код, а выбор, и одним абзацем гасит вопрос «а почему не встроенный ретрай очереди?». Этот вопрос всё равно бы прилетел — только в комментарии, через шесть часов, когда ты уже переключился на другую задачу.

Язык комментария: критикуем код, а не человека

Главное правило англоязычного ревью помещается в одну строку: подлежащее в замечании — код, а не автор. На практике это значит выкинуть you везде, где вместо него можно назвать переменную, функцию или файл.

Разница не косметическая. You forgot — утверждение о человеке и его небрежности; на такое хочется оправдываться. This file handle is never closed — утверждение о факте; на такое хочется ответить коммитом. Один и тот же баг, два разных разговора.

Второй приём — вопрос вместо приговора. Вопрос оставляет автору место объясниться, а тебе — право ошибаться. Он мог знать о коде что-то, чего не знаешь ты: например, что этот «лишний» лок стоит там из-за фонового джоба, который в дифе не виден.

Verdict:  This lock is useless here.
Question: What does this lock protect? The map looks single-threaded to me,
          but I might be missing a caller.

Хеджирование: почему смягчение — это не слабость

Хеджирование (hedging) — смягчающие обороты, которые превращают приговор в предложение обсудить. Русскоязычные разработчики часто видят в них лицемерие: зачем писать I'd suggest, если я точно знаю, что так лучше? Логика тут другая. Хедж говорит не о твоей неуверенности в факте, а о том, что кнопку merge жмёт автор PR, и решение в его ветке принимает он.

А ещё хедж — дешёвая страховка. Написал This is wrong, оказался неправ — отыгрывать назад неловко, потому что ты уже вынес вердикт. Написал I may be missing something, but this looks like it could double-count — и оговорка просто сбылась. Разговор идёт дальше, лицо цело, никто не считает вслух, сколько раз ты ошибся за квартал.

Could we extract this into a helper? It shows up in three places now.
I'd suggest moving the validation before the DB call.
What do you think about using a named constant here?
It might be worth adding a test for the empty list case.
I may be missing something, but does this handle a null `user`?
Have we considered caching this? It runs on every request.
This might be out of scope for this PR, but the same bug exists in `orders.py`.

Выучи наизусть пять конструкций, и на них уедет девять комментариев из десяти: Could we… (мы вместе, а не «ты обязан»), I'd suggest… (мнение, не закон), It might be worth… (стоило бы), Have we considered… (мягко напомнить про вариант), I may be missing something, but… (та самая страховка).

Обратная сторона тоже есть: хеджировать всё подряд нельзя. Замечание про SQL-инъекцию, завёрнутое в maybe it might be worth possibly considering, автор пролистает вместе с придирками к именам переменных. Дальше — про то, как проговаривать вес.

Маркеры важности замечания

Ссорятся в ревью чаще не из-за грубости, а из-за того, что непонятен вес замечания. Ревьюер обронил «мне кажется, тут лучше другое имя» — автор прочитал требование, три часа переименовывал по всему модулю и пришёл в чат с вопросом, зачем это было нужно. Бывает и зеркально: про конкатенацию SQL написано так же мягко, как про пробелы, автор счёл вкусовщиной и смерджил. Лечится одним словом в начале строки. Соглашение неформальное, но узнаваемое почти везде:

nit: typo in the comment — "recieve" → "receive"
optional: a small docstring here would help future readers
suggestion: this loop could be a dict comprehension, up to you
question: is `retries=5` a product requirement or just a guess?
blocking: this builds SQL by string concatenation — it takes user input
          straight from the query string
praise: nice catch on the timezone here, that bug was hard to see

praise: выглядит как реверанс, но работает. Ревью из одних замечаний читается как «всё плохо», даже если замечаний три штуки на четыреста строк — автор видит только красные пометки и не видит, что остальное ты одобрил молча. Одна строка похвалы стоит десять секунд и меняет тон всей ветки.

Таблица: резко → принято

Слева — фразы, которые в обратном переводе на русский звучат совершенно нормально, а в английском ревью прилетают как пощёчина. Справа — то же самое содержание, ноль потерь смысла.

Рецепт правой колонки везде один: вместо оценки — конкретный факт («runs a query per row»), вместо приказа — решение, оставленное автору. Заодно правая колонка полезнее: «obviously this won't scale» нечего чинить, а «10k rows — 10k queries» уже подсказывает join.

Как отвечать на ревью

Отвечать надо на каждый комментарий. Молчаливая правка кажется экономией времени, но ревьюер, вернувшись к PR, не знает, учёл ты замечание или проигнорировал, и лезет перечитывать диф целиком. Минимальный набор ответов:

Good catch, thanks — fixed in a1b2c3d.
Done in 4f5e6d7.
Fixed, thanks!
You're right, I misread the docs. Reverted to the built-in helper.
Ah, I see what you mean now. Updated.

Good catch — дежурная реакция на найденную ошибку: благодарно и без самобичевания. А хеш коммита рядом (fixed in a1b2c3d) экономит ревьюеру минуту поиска по дифу — мелочь, которую замечают.

Отдельный навык — не согласиться и не поссориться. Схема из трёх шагов: признай позицию собеседника, назови причину, оставь дверь открытой.

I'd rather keep it as is, because the caller already validates the input
and a second check would drift out of sync. Happy to add it if you think
it's worth the duplication.

That's a fair concern. I'd suggest we handle it in a follow-up: it needs
a schema change, and this PR is already large. I've opened #533 to track it.

Если спор встал — переписку не продолжают. Зовут третьего или переходят в голос:

We seem to see this differently. Could we get a second opinion from @maria,
who wrote the original module?

I think we're going back and forth on this. Want to jump on a quick call?
I'll write up whatever we decide here afterwards so it's not lost.

Последняя фраза не вежливость, а страховка. Всё, о чём договорились голосом, возвращается в PR текстом — иначе через полгода новый человек придёт с тем же вопросом, и никто уже не вспомнит, почему код такой.

Вердикты и просьбы о ревью

Финальных состояний PR на GitHub три: approve, comment, request changes. Вокруг них есть устойчивые сокращения:

• LGTM — looks good to me, всё нравится, апрувлю. Иногда пишут LGTM with nits — апрув, но есть мелочи на твоё усмотрение.
• ship it — неформальное «выкатывай», ровно то же, что LGTM.
• PTAL — please take another look, «посмотри ещё раз», после того как ты внёс правки.
• Draft или WIP (work in progress) — PR не готов к мерджу, нужен ранний фидбек по подходу, а не построчное ревью.

LGTM, nice cleanup. Two nits inline, feel free to ignore them.
Approving so I don't block you — please address the blocking comment before merging.
Requesting changes: the migration drops a column that the reporting job still reads.

Просьба о ревью — короткая и без давления, зато с оценкой объёма. «Посмотри, там 60 строк конфига» соглашаются посмотреть между делом, «посмотри мой PR» откладывают на потом:

@anna could you take a look when you get a chance? It's a small one,
~60 lines, mostly config.

Hi @team, this one is blocking the release on Thursday. Any chance
someone could review it today?

Gentle bump on this one — no rush if you're heads-down, just want to
make sure it didn't fall off the list.

Gentle bump и Following up on… — стандартные вежливые напоминания, а no rush if… снимает давление.

Кейс из реального проекта: жизнь одного PR от описания до мерджа

Команда из пяти человек, Берлин и Буэнос-Айрес, пересечение по времени — четыре часа в день. Разработчик Дмитрий чинит баг с двойным списанием бонусов. Смотри не на код, а на то, как двое договариваются, ни разу не увидев друг друга.

Шаг 1. Дмитрий открывает PR.

fix(loyalty): prevent double redemption on concurrent checkout

## What
Adds a row-level lock when redeeming loyalty points, so two parallel
checkout requests cannot spend the same points twice.

## Why
Closes #712. Support reported 6 cases last month where a customer ended
up with a negative point balance. All of them came from double-clicking
the "Pay" button.

## How
`SELECT ... FOR UPDATE` on the balance row inside the existing
transaction. I also looked at an optimistic version column, but that
would need a migration and a retry loop in the caller, and the
contention here is very low — one row per user, a few times a day.

## How to test
`pytest loyalty/test_redemption.py -k concurrent`. The new test spawns
two threads redeeming the same points; only one should succeed.

## Risks
- Holds a row lock for the duration of the checkout transaction (~50ms).
- No migration. Revert is safe.

Шаг 2. Ревью от Анны. Пять комментариев, у каждого маркер, и первый из них — похвала. Настоящая находка спрятана во втором.

praise: thanks for writing the concurrency test, those are painful to
        get right and this one is really readable.

blocking: the lock is taken after `points_needed` is computed from a
          separate read. Between the read and the lock the balance can
          change, so the check can pass on stale data. Could we move the
          read inside the locked block?

question: what happens if the checkout transaction is rolled back later —
          is the lock released before we send the confirmation email?

nit: `qs` → maybe `balance_qs`? Took me a second to see what it was.

optional: the magic number 3 on line 88 could be a named constant, but
          it's only used once so up to you.

Шаг 3. Ответы Дмитрия. Одно замечание принято, одно объяснено, одно отклонено — и всё это без единого «ты не понял».

> blocking: the lock is taken after `points_needed` is computed...

Good catch — that's a real race, the check was reading a stale balance.
Moved the read inside the lock in 9ac41f2 and extended the test to cover
it (it fails on the previous commit, I checked).

> question: what happens if the checkout transaction is rolled back...

The email is sent from an `on_commit` hook, so nothing is sent if the
transaction rolls back. I added a comment there since it wasn't obvious.

> optional: the magic number 3 on line 88...

I'd rather keep it inline — it's the HTTP retry count and it lives right
next to the client config, so a constant would move it away from its
context. Happy to change it if you feel strongly.

PTAL when you have a moment.

Шаг 4. Апрув.

LGTM. Fair point on the constant, keeping it inline is fine.
Nice work on the race — that one would have been very hard to debug in prod.

Восемь сообщений, полдня, гонка найдена и закрыта тестом. Теперь поищи в этом обмене слова wrong, bad и обвинительное you — их там нет ни одного. Мягкая форма, жёсткое содержание: именно так это и работает.

Типичные ошибки

Приказной тон

«Перепиши», «убери» в русском ревью — рабочая интонация. Дословный английский императив без смягчения — распоряжение начальника подчинённому, и получает его человек, который тебе не подчиняется.

Как НЕ надо:
Rewrite this.
Remove it.
Don't do this.
Use a map.

Как надо:
Could we restructure this? I left a suggestion below.
I think this can go — it's not called anywhere anymore.
A map would give us O(1) lookups here. Worth it?

«Why did you do this?»

Формально — обычный вопрос. Читается как «ты вообще зачем это сделал?». Виноваты две вещи сразу: you в фокусе и прошедшее время, вместе они превращают строчку в разбор полётов. Спрашивай о коде, а не о поступке.

Как НЕ надо:
Why did you add this dependency?

Как надо:
What does this dependency give us over the stdlib version?
Could you walk me through what this block does? I might be missing context.

Избыточные извинения

Обратная крайность, и я сам через неё прошёл: боишься показаться грубым — и топишь замечание в извинениях. Вежливее оно не становится, только длиннее, а сквозь три sorry уже не видно, что там вообще нашли. Хеджирование — не самоуничижение.

Как НЕ надо:
Sorry, maybe it's a stupid question and sorry for bothering you, but I'm
not sure, maybe I'm wrong, sorry — is this variable used anywhere?

Как надо:
question: is this variable used anywhere? I couldn't find a reference.

Одного хеджа хватает. I couldn't find a reference уже говорит «возможно, я плохо искал» — и ни одного sorry для этого не понадобилось.

Спор в комментариях вместо созвона

Три круга по одной ветке — предел. Дальше растёт только раздражение: интонации в тексте нет, а усталость есть, и каждая следующая реплика читается резче предыдущей, хотя формулировки не менялись. На четвёртом круге зови в голос — и не забудь вернуть решение в PR текстом.

«It doesn't work» без деталей

Ревьюер выкачал ветку, запустил, не завелось — и пишет одну строку. Дальше автор ведёт допрос: какая ОС, какая команда, что в логах. Каждый раунд — полсуток, если вы в разных часовых поясах. Даже внутри своего PR отчёт о поломке содержит команду, окружение и вывод.

Как НЕ надо:
It doesn't work on my machine.

Как надо:
I can't get this to start locally. On macOS 15, Python 3.12, after
`make dev` I get:

    ImportError: cannot import name 'RedisLock' from 'loyalty.locks'

Do I need to re-run migrations, or is a new env var required?

Итог

Pull request — текст, который читают чаще, чем код внутри него. Заголовок в повелительном наклонении, по формату conventional commits, говорит, что меняется. Описание по схеме What / Why / How / How to test / Risks закрывает всё, чего не видно в дифе, и снимает с ревьюера десяток вопросов ещё до того, как он их задал.

В комментариях работают два правила. Подлежащее — код, а не человек. Вес замечания проговаривается словом: nit:, question:, suggestion:, blocking:, optional:, praise:. Хеджирование (Could we…, I'd suggest…, I may be missing something, but…) — не робость, а способ оставить решение автору и дёшево отыграть назад, когда ошибся ты. И отвечать надо на всё: Good catch, Done in a1b2c3d, а при несогласии — признать позицию, назвать причину, позвать третьего.

В главе 8 уходим от кода к тексту вокруг него: issue и баг-репорты, на которые отвечают, и документация, которую дочитывают.