company_banner

Процесс ревью кода в hh.ru

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



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


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


    • В 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. Делитесь в комментариях своими правилами, рекомендациями и полезными юзкейсами по процессу ревью
    • +34
    • 15,8k
    • 1
    HeadHunter
    164,20
    HR Digital
    Поделиться публикацией

    Похожие публикации

    Комментарии 1

      0
      Отличная статья. Александр, спасибо огромное.

      Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

      Самое читаемое