Процесс ревью кода в HH.RU06.09.2018 11:47

Общие положения

• В HH.RU ревью проходит в формате пулл-реквестов на Гитхабе.
• Ревью и пулл-реквесты обязательны, даже если в рамках задачи меняется одна строчка или один символ в коде.
• У каждой задачи должен быть, как минимум, один ответственный ревьюер, он указывается в задаче в качестве ревьюера и несёт ответственность за код, как и автор. Не запрещено и приветствуется участие в ревью других людей.
• Ответственность провести задачу через ревью лежит на авторе задачи.
• Любые изменения после ревью, например, правки во время тестирования, точно так же должны быть проревьюены.

Цели ревью

• Распространение опыта и знаний между разработчиками.
• Поддержка принципа — изменения не должны ухудшать уже существующий код, они должны его улучшать.
• Исправление ошибок в логике и ошибок связанных с безопасностью, корректную обработку исключений.
• Улучшение качества кода, maintainability и архитектуры проекта.
• Улучшение читабельности кода.
• Улучшение покрытия тестами и тестирование на нужном уровне.
• Улучшение документации.
• Соответствие изменений требованиям в задаче.
• Предотвращение появления технического долга или технического налога.
• Продуманный дизайн API, где API — это любой модуль с внешним интерфейсом. Например, что ресурс в сервисе имеет продуманный URL, удобный формат JSON, корректные коды ответов в разных случаях и так далее.
• Корректная история коммитов в Гите, с отражающими суть сообщениями, без временных коммитов.

Подготовка к ревью

• Перед публикацией пулл-реквеста 2–3 раза прочитайте свой код: с большой вероятностью вы сами найдёте там ошибки, что сэкономит время ревьюеру. Проверьте, что нет ошибок, которые могут быть выявлены автоматически — ошибки в стиле кода, упавшие тесты. Удостоверьтесь, что в коде нет временных комментариев и отладочного кода, а также проверьте соответствие вашего кода принятым стайл-гайдам.
• Если создаёте пулл-реквест в процессе работы над задачей, пропишите в заголовке пометку »[WIP]» и поставьте метку «work in progress». Когда работа закончена, уберите метки и проинформируйте об этом отдельным комментарием, чтобы заинтересованные лица узнали, что код можно смотреть.
• Будьте готовы показать ревьюеру мини-демо по задаче.
• Отдавайте на ревью небольшие порции кода. 200–300 строк изменений — предел для эффективного ревью.
• Оформляйте независимые изменения отдельными коммитами.
• Описывайте коммиты короткими и информативными сообщениями.
• Рефакторинг делайте отдельным коммитом, так легче увидеть изменения в логике, суть задачи.
• Форматирование кода делайте отдельным коммитом до основных изменений, или даже отдельным пулл-реквестом.
• Не коммитьте незначащие изменения. Например, добавление переносов строк в файле, в котором вы больше ничего не меняли.
• В заголовке пулл-реквеста коротко и информативно опишите суть изменений.
• Опишите в пулл-реквесте проблему и решение. Опишите альтернативные варианты решения и почему выбрано текущее решение. Если изменения касаются интерфейса, приложите скриншоты «до» и «после».
• В пулл-реквесте дайте ссылку на задачу, а в задаче ссылку на пулл-реквест.
• Иногда полезно обсудить выбранное решение с ревьюером до написания кода. Это поможет найти оптимальный вариант решения задачи и упростит процесс ревью.

Выбор ревьюера

• Отдавайте задачу на ревью специалисту из соответствующей области.
• Если с каким-то кодом работает несколько команд, то полезно для распространения знаний выбирать ревьювера из другой команды.
• Сотрудник на испытательном сроке не может быть ответственным ревьюером, но может участвовать в процессе ревью.
• Не отдавайте все ваши задачи на ревью одним и тем же людям — просите ревью у разных людей.
• Если в задаче затронут нетривиальный участок кода, в котором разбирается определённый человек, покажите изменения ему, независимо от того, кто ответственный ревьюер. Также помните, что у каждого репозитория есть меинтейнеры, они больше других знают об этом коде и курируют разработку.
• В остальном ревьюер выбирается произвольно, по обоюдному согласию сторон. Для помощи в выборе используйте рекомендации на Гитхабе, основанные на «блейме» затронутого кода.
• После выбора ревьюера укажите его в качестве Assignee в пулл-реквесте. Список всех пулл-реквестов, которые на вас назначены.

Процесс ревью, общие положения

• Весь код общий, нет таких понятий как «мой код» или «твой код». Это значит, что за любую строчку кода отвечает каждый разработчик, а не только тот, кто эту строчку написал.
• Создавайте атмосферу взаимопонимания, будьте вежливы и дружелюбны, цените и уважайте друг друга, не навязывайте своё мнение. Прислушивайтесь к мнению оппонента и не стойте на своём из принципа. Не превращайте ревью в отрицательный опыт для коллег.
• Задавайте вопросы, если что-то не понятно. Аргументируйте и конкретизируйте вопросы и ответы. Автору кода может быть не очевидно, что подразумевается под вопросом «почему так?», а ревьюверу непонятна аргументация ответа «не согласен».
• Не растягивайте процесс ревью, экономьте время, оперативно реагируйте на комментарии, обсуждайте устно, не провоцируйте длинные дискуссии и не разводите «холиваров». Если вы не можете быстро прийти к консенсусу обратитесь за помощью к коллегам, к меинтейнеру или руководителю функционального направления.
• Если задача не на пару строк, потратьте с ревьюером 10–15 минут на совместный просмотр кода, полезно сделать это даже до создания пулл-реквеста. Обсудите суть задачи и решения, посмотрите как было и стало в работающем окружении. Это поможет ревьюеру лучше погрузиться в контекст задачи. Большинство комментариев останутся ненаписанными, что сэкономит всем время.
• После устного обсуждения всегда описывайте принятое решение и доводы в пользу него в пулл-реквесте. Ответственность за описание лежит на авторе кода.
• Если в коде встречаются однотипные ошибки, ревьюеру следует акцентировать на этом внимание автора кода в первом или втором комментарии, не комментируя каждое вхождение, а автору анализировать код на предмет однотипных ошибок перед публикацией исправления.
• Хвалите за удачные решения автора и предложения ревьювера.
• Оставляйте комментарии к изменениям в самом пулл-реквесте, а не к отдельным коммитам. Таким образом, вся переписка будет в одном месте, в том числе после ребейза.
• Отвечайте на комментарии в тех же ветках обсуждений, где они начаты. Если комментарий относится ко всему пулл-реквесту или в одной ветке несколько комментариев, цитируйте комментируемое сообщение. Чтобы из письма перейти на устаревшую ветку обсуждения вместо ссылки «view it on GitHub» используйте ссылку «in path/to/file».
• Если в процессе ревью приняты какие-либо кардинальные решения с точки зрения стандартов кодирования или иных оформленных договорённостей, на автора кода возлагается обязанность записать эти решения в соответствующих документах.
• Ревью не только про код, а про задачу целиком. Не относитесь к требованиям в задаче или макетам как к истине в последней инстанции — все совершают ошибки и никто не может учесть все нюансы. Свежий взгляд и дельные предложения только пойдут продукту на пользу.

Процесс ревью со стороны автора кода

• Задача не завершена, пока не прошла ревью, тестирование и выпуск на «прод».
• Отвечайте на все комментарии ревьюера, не оставляйте комментарии без ответа, независимо от того приняли вы их или нет.
• Задумывайтесь над вопросами, которые возникают или могут возникнуть во время ревью. Возможно, участку кода не хватает комментария или код лучше написать более явно.
• Не исправляйте существующие коммиты «амендом» (git commit --amend), вместо этого вносите изменения отдельными коммитами, по одному на каждое исправление. Исправление существующих коммитов сильно затрудняет процесс ревью, так как приходится смотреть весь код заново.
• После добавления нового коммита к пулл-реквесту, напишите отдельным комментарием сводку об изменениях, чтобы заинтересованные лица были в курсе. Это касается как исправлений во время ревью, так и исправлений во время тестирования.
• После ревью, перед тем, как отправить задачу в тестирование, перепишите историю коммитов (git rebase --interactive), внеся в отдельные коммиты соответствующие исправления и удалив временные коммиты. Обратите внимание на опцию --autosquash для упрощения процесса.

Процесс ревью со стороны ревьюера

• Если в момент просьбы поревьюить вы заняты, сообщите когда именно вы приступите к ревью.
• Не требуйте, а предлагайте внести изменения.
• Комментируйте код, а не человека, который его написал.
• Вы берёте на себя ответственность за написанный код, подходите к ревью соответствующе, но это не значит, что автор кода должен неукоснительно выполнять все ваши требования. Помните, многие вещи можно сделать по-разному.
• Преследуйте цели ревью и предлагайте правки по существу. Не настаивайте на некритичных изменениях. Указывайте важность исправлений в комментарии.
• Если вы нашли серьёзную проблему, приостановите ревью и дождитесь, пока автор её исправит. Вероятнее всего, после исправления всё равно потребуется ревьюить заново.
• Обращайте внимание не только на то, что было изменено, но и на то, что не было изменено. Поищите и покажите автору код, который должен был быть изменён, но не был (забытые импорты или неиспользуемые классы, использование отрефакторенного кода в другом месте и прочее).
• После внесения изменений автором кода проревьюйте исправления и повторно просмотрите весь пулл-реквест. Возможно, с учётом исправлений у вас появятся новые замечания или вопросы.
• Не тратьте время на ревью кода, который не менялся в рамках задачи. Выделение части кода в функцию считается за изменение. Перенос кода «как есть» за изменение не считается. Реформат кода в затронутом файле на усмотрение автора.
• Ревьюер не правит самостоятельно код в ветке, потому что ему так хочется или проще. Изменения вносит автор кода.
• Если взялись ревьюить, старайтесь не затягивать и не переключаться на другие задачи в ущерб текущей

Использованные материалы и ссылки по теме

P.S. Делитесь в комментариях своими правилами, рекомендациями и полезными юзкейсами по процессу ревью