Docstrings, комментарии, README, changelog, документация API
Документация — не сопроводительный текст к коду, а часть контракта. Если контракт изменился, а документация нет, вы выпустили ломающее изменение молча.
Вы научитесь понимать, какая документация обязательна в конкретном PR, а какая — лишняя работа; замечать изменения контракта, которые автор не отразил нигде; и различать комментарий, объясняющий решение, от комментария, пересказывающего код.
Требовать документацию на всё — верный способ получить формальные заглушки, которые устареют через месяц. Полезное правило: документируется то, что пересекает границу.
| Что меняется в PR | Что обязано измениться вместе |
|---|---|
| Публичная функция или класс библиотеки | Docstring: назначение, параметры, возврат, исключения |
| HTTP-эндпоинт | Схема запроса и ответа, коды ошибок |
| Формат конфигурации, новая переменная окружения | README или файл-пример конфигурации |
| Порядок запуска, зависимости, миграции | README, инструкция по развёртыванию |
| Поведение, видимое пользователю | Changelog |
| Ломающее изменение | Changelog с пометкой и путём миграции |
| Приватная функция внутри модуля | Обычно ничего — хватает имени и типов |
Обратная сторона правила: если приватная вспомогательная функция требует абзаца объяснений, это сигнал, что она делает слишком много.
Проверьте себя. Возьмите открытый PR и определите по таблице, что в нём должно было обновиться. Всё ли на месте?
Частая ошибка. Требовать docstring на каждую функцию. Через полгода половина из них будет описывать поведение, которого уже нет.
Полезный docstring отвечает на вопросы, ответа на которые нет в сигнатуре: что означают значения аргументов, что происходит в исключительных случаях, есть ли побочные эффекты.
# ❌ пересказывает имя функции
def calculate_discount(price, discount):
"""Calculates discount."""
# ✅ описывает контракт: единицы измерения, границы, поведение при ошибке
def calculate_discount(price: Decimal, discount: float) -> Decimal:
"""Возвращает цену после скидки.
Args:
price: исходная цена в рублях, строго положительная.
discount: доля скидки в диапазоне [0, 1]; 0.1 означает 10 %.
Returns:
Цена после скидки, округлённая до копеек вниз.
Raises:
ValueError: если price <= 0 или discount вне [0, 1].
"""Обратите внимание, что именно добавляет ценность: discount — доля, а не проценты (частый источник ошибки в сто раз), округление идёт вниз, границы диапазона включены. Ни одного из этих фактов в сигнатуре нет.
С аннотациями типов документация становится короче: не нужно повторять типы словами. user_id: int уже сказано в сигнатуре, повторять «Args: user_id (int): идентификатор пользователя» — шум.
Формат (Google, NumPy, reStructuredText) — вопрос соглашения в проекте, а не предмет обсуждения в PR. Если формат единый, проверять его должен линтер.
Проверьте себя. Найдите функцию с числовым параметром и проверьте, понятны ли из документации единицы измерения — секунды или миллисекунды, доля или проценты.
Частая ошибка. Docstring, дублирующий сигнатуру. Это не документация, а работа, которая устареет.
# ❌ дублирует код и рассинхронизируется при первой правке
# увеличиваем счётчик на единицу
counter += 1
# ✅ объясняет неочевидное решение
# Ретраим 3 раза: платёжный шлюз возвращает 502 при плановом
# переключении реплик, оно длится до 8 секунд. См. INC-4412.
for attempt in range(3):
...Ценность второго комментария в том, что он останавливает будущего разработчика от «упрощения». Без него кто-нибудь уберёт ретраи как избыточные, и инцидент повторится.
Что стоит комментария: неочевидный выбор из нескольких вариантов, обход чужого бага со ссылкой, компромисс с указанием причины, ограничение, которое неоткуда узнать из кода.
Отдельно про пометки: TODO без имени и контекста живёт вечно. Полезная форма — TODO(PROJ-123): убрать после перехода на v2 API, где есть тикет и условие снятия.
Проверьте себя. Посчитайте TODO в проекте и посмотрите, у скольких есть ссылка на задачу.
Частая ошибка. Просить комментарий там, где нужно переименование. Если функцию приходится объяснять — сначала попробуйте назвать её точнее.
Описание PR читают дважды: ревьюер сейчас и разработчик через год, когда git blame приведёт его к этому коммиту. Второе прочтение важнее.
Полезное описание отвечает на три вопроса: какую проблему решаем, почему выбран этот способ, что нужно проверить при проверке. Отсутствие описания — законный повод не начинать ревью: без контекста вы проверяете код, а не решение.
❌ «Фикс бага»
✅ Заказы от 5000 ₽ облагались доставкой (INC-820).
Причина: сравнение `>` вместо `>=` на границе порога.
Проверить: заказ ровно на 5000 ₽ — доставка 0 ₽.
Миграции нет, откат безопасен.Проверьте себя. Откройте случайный PR полугодовой давности и попробуйте понять по описанию, зачем он был нужен.
Частая ошибка. Считать, что тикет заменяет описание. Тикет говорит, что просили; PR должен объяснять, что и почему сделано.
Самая дорогая пропущенная документация — необъявленное ломающее изменение. Оно выглядит в диффе как обычная правка.
Что считается ломающим: переименование или удаление поля ответа API, сужение допустимых значений, изменение кода ошибки, новое обязательное поле в запросе, смена формата даты, изменение семантики при том же имени, удаление переменной окружения со значением по умолчанию.
class UserResponse(BaseModel):
- name: str
+ full_name: strВ диффе это две строки. Для мобильного приложения, которое останется в проде ещё полгода, — сломанный экран профиля. Ревьюер обязан спросить: кто потребители этого поля и как они узнают об изменении.
Ответ «мы поправим фронтенд в том же релизе» подходит, только если других потребителей нет и они выкатываются одновременно. Во всех остальных случаях нужен переходный период с обоими полями.
Проверьте себя. Для последнего изменения в API вашего проекта перечислите всех известных потребителей. Список получился полным?
Частая ошибка. Считать ломающими только изменения сигнатуры. Изменение смысла при неизменной сигнатуре опаснее — его не заметит даже типизация.
Новая публичная возможность интеграции. Дифф на 60 строк, документации в PR нет.
+@app.post("/api/webhooks/subscribe")
+def subscribe(url: str, events: list[str], secret: str | None = None):
+ """Подписка на вебхуки."""
+ sub = Subscription(url=url, events=events, secret=secret)
+ db.save(sub)
+ return {"id": sub.id}
+
+
+def send_webhook(sub: Subscription, event: str, payload: dict):
+ body = json.dumps({"event": event, "data": payload, "ts": time.time()})
+ sig = hmac.new(sub.secret.encode(), body.encode(), "sha256").hexdigest()
+ requests.post(sub.url, data=body, headers={"X-Signature": sig}, timeout=5)Что видит ревьюер. Это публичный интерфейс для внешних интеграторов — то есть случай, когда документация не пожелание, а часть работы. При этом:
Docstring «Подписка на вебхуки» не сообщает ничего сверх имени. Неизвестно, какие значения допустимы в events, что происходит при повторной подписке на тот же URL, обязателен ли secret.
Формат самого вебхука не описан нигде: интегратору неоткуда узнать структуру data, значение ts (секунды или миллисекунды, UTC или локальное), алгоритм подписи и то, от какой именно строки она считается. Без этого проверить подпись на своей стороне невозможно.
Поведение при сбое не описано и, судя по коду, не реализовано: одна попытка с таймаутом 5 секунд, результат игнорируется. Интегратор будет считать, что доставка гарантирована.
И баг, найденный попутно: sub.secret необязателен, а send_webhook вызывает .encode() без проверки на None.
Комментарии в PR:
webhooks.py:11· blockersecretнеобязателен, но вsend_webhookвызываетсяsub.secret.encode()—AttributeErrorдля любой подписки без секрета. Либо сделайте поле обязательным, либо не подписывайте такие запросы.
webhooks.py:1· major Это публичный контракт для внешних интеграторов, а описания формата нет. Нужен раздел в документации API: список допустимых значенийevents, структура тела вебхука, единицыts, алгоритм подписи и то, от какой строки считается HMAC — иначе подпись невозможно проверить на стороне получателя.
webhooks.py:11· major Одна попытка доставки, результат не проверяется. Опишите в документации гарантии явно: «доставка не гарантирована» — допустимая позиция, но интегратор должен о ней знать до того, как построит на этом биллинг. Если гарантии нужны — это отдельный тикет на очередь с повторами.
webhooks.py:2· minor Docstring повторяет имя функции. Полезнее описать, что произойдёт при повторной подписке на тот же URL — заменится, задублируется или вернётся ошибка.
webhooks.py:1· question Что с приватными адресами? Сейчас можно подписаться наhttp://169.254.169.254/...и заставить сервис ходить во внутреннюю сеть. Это SSRF — вероятно, нужен отдельный тикет, но давайте зафиксируем.
Чем закончилось. Секрет стал обязательным, появился раздел документации с примером тела вебхука и готовым фрагментом проверки подписи на трёх языках, в описании явно записано «at-least-once, до 5 попыток с экспоненциальной задержкой» — гарантии заодно решили реализовать. Вопрос про SSRF ушёл отдельным тикетом с высоким приоритетом.
Отметьте: замечание про документацию здесь оказалось major, а не nit. Для публичного контракта отсутствие описания — такой же дефект, как отсутствующая проверка.
ГРАНИЦЫ всё, что пересекает границу модуля/сервиса, описано
КОНТРАКТ единицы измерения, диапазоны, побочные эффекты, исключения
СЛОМ ломающие изменения помечены, путь миграции описан
README обновлён, если изменились запуск, конфигурация, зависимости
CHANGELOG отражает изменения, видимые пользователю
КОММЕНТАРИИ объясняют «почему», содержат ссылку на тикет или инцидент
TODO с номером задачи и условием снятия
ОПИСАНИЕ PR проблема, выбранное решение, что проверить
АКТУАЛЬНОСТЬ документация рядом с изменённым кодом всё ещё правдиваКак контракт API проектируется и версионируется, чтобы ломающих изменений было меньше, — в теме Дизайн API курса Pro.
Ключевая мысль: документация проверяется не на наличие, а на правдивость. Устаревшее описание вреднее отсутствующего — ему верят.
Далее: Soft skills в code review