Описание процесса код-ревью¶

У нас весь код должен проходить обязательное код-ревью. Процесс код-ревью преследует несколько целей:

Улучшение качества продукта за счёт раннего обнаружения скрытых багов.
Контроль качества кода, чтобы он был поддерживаем на протяжении многих лет.
Обмен знаниями между разработчиками.

Ревью проводятся с помощью инструмента Upsource по адресу: http://upsource.libicraft.com. Также рекомендуется установить плагин для IDE Upsource Integration, которым довольно удобно пользоваться.

Мы практикуем post-commit review — просмотр кода происходит после того, как он попал в центральный репозиторий. Это не означает, что код может “лежать” там непросмотренным бесконечное время. Нужно придерживаться определённых в этом документе сроков и стараться завершить ревью как можно быстрее. Так работа будет вестись с кодом, который ещё лежит в оперативной памяти мозга и не потребуется тратить время на многократную загрузку с жёсткого диска 🙂

Чтобы процесс код-ревью не отодвинул основную работу по выполнению задач на второй план, рекомендуется выделять для ревью определённое время в течение дня (например, вечером перед уходом домой) не больше часа и заниматься просмотром кода коллег только в это время. Но к исправлению замечаний по своему коду следует относится как к исправлению своих багов и отдавать должный приоритет.

Код-ревью — рутинный процесс, который нужно научиться выполнять правильно, чтобы не усложнять жизнь ни себе, ни коллегам. Upsource не позволяет польностью зарегламентировать процесс. Поэтому следует отнестись к освоению алгоритма серьёзно (но без излишнего фанатизма 🙂 )

1. Коммит кода и создание код-ревью¶

После того, как фича сделана или баг исправлен и код запушен в центральный репозиторий, разработчик должен запостить код-ревью в upsource. Для этого нужно:

Нажать Create review напротив соответствующего коммита.
На странице созданного ревью во вкладке Revisions добавить дополнительные коммиты, если это необходимо.
Во вкладке Overview добавить ревьюеров и наблюдателей, если необходимо.

Ревью необходимо постить сразу после пуша в центральный репозиторий.

Не нужно постить ревью на изменения, которые содержат в себе мелкие исправления код-стайла.

Как запостить хороший код-ревью¶

Коммит должен быть небольшой, затрагивать немного файлов и заключать в себе минимальное количество реализованного функционала. Это правило касается любого хорошего коммита, а не только в контексте процесса код-ревью.

Большие ревью очень трудно проводить. Они приводят либо к просмотру ревьюером “спустя рукава” по диагонали, либо к затягиванию процесса.
Прежде, чем коммитить код, разработчик должен сам просмотреть его и проверить:
- Не попадает ли в коммит лишний файл?
- Соответствует ли изменённый код стайл-гайду? Большое количество очевидных ошибок по код-стайлу только вызовет раздражение у ревьюеров и захламит ревью комментариями. Никому не нравится проверять код-стайл, уважайте труд коллег.
- Не осталось ли дебаг-кода (console.log, debugger и т.д.)?
- Не нужно ли прокомментировать неочевидные куски кода или написать док-блоки там, где это необходимо по гайдам?
Примечание

Для удобного просмотра изменений перед коммитом следует использовать функцию Commit Changes в WebStorm/IDEA, а не мнить себя крутым хакером, которому “в командной строке удобнее в стопицот раз”.
Следует написать понятный комментарий к коммиту с описанием того, о чём исправления, с указанием номера issue в YouTrack (если есть).
Подходить к выбору ревьюеров с умом, а не по принципу “добавлю-ка всех на всякий случай”. Помогут следующие ориентиры:
- Лучше добавить разработчика, который раньше занимался этой или подобной задачей или куском.
- Хороший кандидат на роль ревьюера — изначальный автор файлов, в которых были изменения (можно посмотреть по аннотации и Upsource вроде умеет подсказывать).
- Новички должны всегда добавлять в ревью опытных разработчиков как минимум в течение месяца.

2. Просмотр и комментирование кода ревьюером¶

Ревьюер получает уведомление на почту и должен просмотреть, прокомментировать и дать своё заключение. Это можно сделать как в веб-интерфейсе Upsource, так и в IDE (если стоит плагин).

Максимальный срок на просмотр - 2 рабочих дня, но если нет никаких особых причин, ревью следует проводить в течение одного дня.

Если ревьюер понимает, что он перегружен и ревью может провести кто-нибудь другой без ущерба для результата, или считает, что есть более удачная кандидатура на просмотр этого кода, то он может написать соответствующий комментарий в ревью (обязательно!) и удалить себя из списка ревьюеров.

Ревьюер может писать комментарии одного из трёх типов:

Комментарий, требующий исправления дефекта: явная ошибка, код-стайл, комментарий и т.д.
Комментарий, требующий ответа: в сомнительном месте задать вопрос.
Просто реплика с полезной информацией, которая предполагает только ознакомление автора и больше никаких действий с его стороны.

Если есть хотя бы один комментарий первого или второго типа, то по завершению просмотра ревьюер должен отклонить изменения, нажав кнопку 👎 Raise concern, иначе — принять изменения, нажав кнопку 👍 Accept.

Как провести хороший код-ревью¶

Будьте вежливы — во время ревью вы оцениваете/критикуете код, а не автора. В комментариях к ревью запрещено оскорблять друг-друга (напрямую или намёками) и использовать нецензурную лексику. Комментарии не должны быть агрессивны или вызывать в ответ агрессию автора («А это ещё что за убогая фигня?» — плохой комментарий).
Будьте точны, не говорите намёками — «Тут ошибка» vs «Потенциальный SQL-injection, надо экранировать».
Используйте функцию выделения блока кода для комментирования. Она доступна в Side-by-side diff в веб-интерфейсе или в IDE.
Просмотрите и прокомментируйте все файлы в ревью за один подход, чтобы минимизировать количество циклов исправления замечаний автором ревью.
По комментарию должно быть очевидно, требуется ли автору что-то делать или нет.
Если видите непонятный/неочевидный код, напишите комментарий, требующий его упростить или добавить поясняющий комментарий в код.
Если одна и та же ошибка повторяется несколько раз в файле, напишите комментарий к первой с дополнением «ниже тоже».

3. Исправление замечаний и обсуждение¶

Автор должен просмотреть и отреагировать на замечания ревьюера в течение одного рабочего дня. Реакция должна быть следующей:

В ответ на комментарий, требующий исправления дефекта, можно отреагировать следующими действиями:
- Внести соответствующее исправление в код.
- Ответить на комментарий с возражением или просьбой разъяснения.
- Если замечание не является критическим с точки зрения продукта и требует, например, большого рефакторинга, то можно создать сооветствующий тикет в YouTrack (кнопка Create issue) с техническим долгом и дать ссылку в ответе.
При этом галочка Resolved не должна выставляться автором, такие замечания резолвит ревьюер.
В ответ на комментарий, требующий ответа, автор должен что-нибудь ответить :) При этом галочка Resolved также не должна выставляться автором, такие обсуждения резолвит ревьюер.
Информационный комментарий автор должен прочитать и выставить галочку Resolved, если ему всё понятно и нечего сказать, либо ответить с целью продолжить дискуссию. Во втором случае галочку надо выставить, когда дискуссия завершена.

Как делать коммит с исправлениями по ревью¶

Очень желательно, чтобы коммит с исправлениями по ревью содержал сразу исправления по всем замечаниям и только их. Если в файлах, которые нужно закомитить, содержатся посторонние незакомиченные изменения, то лучше до внесения исправлений по ревью, воспользоваться командой git stash или соответствующей функцией в IDE.
Комментарий к комиту должен содержать номер ревью, например:
```
LCB-CR-3 code-style fixes
```
Тогда Upsource сам добавит это изменения в соответствующий ревью. Если этого не сделать, нужно добавлять этот комит в ревью вручную из веб-интерфейса.
Старайтесь не подмешивать в этот коммит посторонние изменения. Это сильно затруднит работу ревьерам.
Не забудьте сделать git push, чтобы ревьюеры увидели изменения :)

4. Подтверждение исправлений ревьюером¶

Когда ревью обновляется новыми коммитами и комментариями, ревьюер должен просмотреть все изменения и ответы. Если ответное действие на его замечание его устраивает, то ревьюер выставляет галочку Resolved у этого комментария/обсуждения. Если не устраивает — пишет соответствующий ответ.

Если ревьюер находит в коде какие-то новые дефекты, то он должен добавить новый комментарий, как на втором этапе.

Ревьюер должен просматривать и резолвить в первую очередь только свои комментарии. Однако, в очевидных случаях или по договорённости он может закрывать дискуссии, инициированные другими ревьюерами, тоже.

Если все свои и комментарии других ревьюеров, с которыми он согласен, помечены Resolved, то ревьюер должен принять изменения, нажав кнопку 👍 Accept. Иначе — отклонить кнопкой 👎 Raise concern, что возвращает процесс ревью к шагу 3.

Сроки подверждения/отклонения изменений ревьюером такие же, как и на этапе комментирования: максимум — 2 рабочих дня, желательно — за 1 день.

5. Завершение ревью¶

Автор может закрыть ревью, нажав кнопку Close review в одном из следующих случаев:

Все ревьюеры приняли изменения (👍 ).
Хотя бы один ревьюер принял изменения (👍 ), а остальные не провели просмотр (второй шаг) в течение 2-х дней.
Хотя бы один ревьюер провёл просмотр (👎 ), автор отреагировал на все замечания (третий шаг), но ревьюеры не подтвердили исправления (четвёртый шаг) в течение 2-х дней.

Ревью нельзя закрыть, если ни один ревьюер не просмотрел его и не поставил свою резолюцию (неважно, положительную или отрицительную). Также перед закрытием должны быть отработаны все замечания, сделанные ревьюерами.

Если ревьюеры молчат¶

Ответственность за своевременное проведение и завершение ревью лежит в первую очередь на авторе.
Если ревьюеры молчат больше одного дня, автор должен напомнить им либо комментарием в ревью, либо по другим каналам.
Если уговоры не помогают и ревьюеры молчат больше 2-х дней, автор должен подключить в ревью начальника (либо начальника начальника, если начальник и есть ревьюер) и сообщить о нарушении.
В общем случае автор не должен иметь незакрытых ревью возрастом более 5 рабочих дней.

Содержание

Вы здесь: