Процесс ревью кода в HH.RU

в 8:29, , рубрики: code review, Git, github, HH, hh.ru, Блог компании HeadHunter, Программирование, разработка, ревью, ревью кода, Совершенный код, хедхантер

Мне на глаза попался документ с правилами и рекомендациями по процессу ревью кода внутри компании. Я решил, что такой полезной информацией надо поделиться с внешним миром. С благословения автора я публикую работу
Процесс ревью кода в HH.RU - 1

Большинство указанных правил носят рекомендательный характер и надо руководствоваться в первую очередь здравым смыслом, а не слепым соблюдением.

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

  • В 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. Делитесь в комментариях своими правилами, рекомендациями и полезными юзкейсами по процессу ревью

Автор: shurik2533

Источник

* - обязательные к заполнению поля

Главная   |  Архив новостей  |   Android  |   Google  |   Apple  |   Microsoft  |   Информационная безопасность  |   Веб – разработка
Публикации RSS  |  Комментарии RSS
https://ajax.googleapis.com/ajax/libs/jquery/3.4.1/jquery.min.js