Перевод статьи «Improving Code Quality using Pull Requests».
Пул-реквесты (PR) немного напоминают написание автоматизированных тестов. Во-первых, тем, что их пользу тяжело измерить, поскольку все проекты разные. В то же время, использование и тех, и других, стало стандартом индустрии. Во-вторых, и к PR, и к тестам относятся с изрядной долей скептицизма. Многие считают, что и те, и другие замедляют работу разработчиков и мешают быстрым поставкам функционала. Наконец, и тесты, и пул-реквесты теряют всякий смысл, если они используются только для проформы.
Что еще любопытно в отношении тестов и пул-реквестов, это то, что они способствуют различным «побочным» улучшениям. Например, тесты (и особенно TDD) могут улучшить качество и дизайн кода. В этом посте я расскажу о лучших практиках пул-реквестов и отдельно сделаю упор на том, как они способствуют различным улучшениям.
Прежде чем начать, давайте разберемся, зачем вообще нужны пул-реквесты. Сильно углубляться не будем, ведь каждая команда сама решает, почему они важны или не важны для нее. Я лишь упомяну, какую пользу они могут принести:
- PR дают автору уверенность в том, что его код достаточно хорош для слияния.
- Они способствуют распространению знаний.
- Пул-реквесты делают нас менее ленивыми.
- Они помогают замечать баги.
Как лучше подготовить пул-реквест
Ограничивайте его размер
Размер это одна из самых больших проблем пул-реквестов, которая влияет на то, будет ли ваш опыт их применения полезным. Когда файлов много, сложно понять, что именно меняется, и заметить проблемы. В зависимости от членов команды большие пул-реквесты обычно приводят к ситуациям двух видов:
- Ревьюеры лишь бегло просматривают файлы и одобряют их, не читая.
- Ревью затягивается, потому что вызывает множественные дискуссии, в результате которых автору приходится фиксить много файлов.
Для уменьшения размера пул-реквестов применяются различные подходы. Ваши действия при этом могут зависеть от типа проекта и циклов релизов.
Например, можно сначала представить частично рабочее решение, а затем, в последующих пул-реквестах, расширить его. Скажем, если вам нужно реализовать таблицу данных с фильтрацией и сортировкой, можно начать с самой таблицы, затем, в других пул-реквестах, добавить сортировку, а в конце – фильтрацию. Или, если у вас единый репозиторий проекта, вы можете сначала представлять изменения API, а затем – изменения UI.
Если вы не хотите мержить частично рабочий функционал в общую ветку, есть и другие подходы. Можно делать пул-реквест в ветку фичи. Для этого вам нужно будет создать ответвление от ветки главного функционала и делать пул-реквесты в это ответвление. Да, с ветками придется повозиться, но в определенных обстоятельствах это может быть хорошим выходом.
Есть и другие способы делить большую проблему на более мелкие, и выбор зависит от конкретного исполнителя. Но сам процесс такого деления улучшает ваши навыки решения проблем. Это, в свою очередь, помогает в разных рабочих операциях, например, при оценке времени и ресурсов, необходимых для выполнения задач.
Следите также за тем, не являются ли большие пул-реквесты симптомом слишком больших задач/историй. Если задачи достаточно маленькие, в указанных подходах редко возникает необходимость.
Отдельный пул-реквест для рефакторинга
Этот пункт тесно связан с предыдущим, но я хотел бы заострить на нем ваше внимание, потому что это хороший подход к разработке, даже если вы не делаете пул-реквестов.
Общее правило гласит: «Сначала сделайте рефакторинг, облегчающий добавление нового функционала, и лишь затем добавляйте новый функционал». Это прекрасный подход, поскольку вместо излишнего технического усложнения вы позволяете новому функционалу «руководить» дизайном системы.
Следуя этому правилу, вы сможете делать более простые PR. Вы автоматически будете разбивать большие пул-реквесты на меньшие. И в то же время, если рефакторинг будет затрагивать много файлов, вы внесете изменения на более ранней стадии.
Я не всегда следовал этому правилу и это часто приводило к болезненным конфликтам слияния, поскольку другие члены команды редактировали те же файлы.
Единственный недостаток этого подхода в том, что вы можете сделать рефакторинг кода, тесно связанного с той фичей, которую реализуете, и по какой-то причине эта фича может исчезнуть из кодовой базы. В результате вы остаетесь с кодом, отредактированным и подготовленным для функционала, который больше не нужен.
Мудро используйте сообщения git-коммитов
Есть еще один общий совет по программированию, подходящий для пул-реквестов: делайте хорошие сообщения коммитов. Они документируют код и все изменения в пул-реквесте.
Воспринимайте это так: вместо создания описания с перечислением вносимых вами изменений, их можно задокументировать в сообщении коммита. А в описании пул-реквеста можно написать другую относящуюся к делу информацию.
Все это имеет и побочный эффект. Как выразился мой коллега, «Написание содержательных сообщений коммитов помогает лучше структурировать задачи и делит ваш код на смысловые части».
Комментируйте сам код
Если у вас есть сомнения касательно реализации фичи, лучше обсудите их с коллегами до того как начнете писать код. Не делайте этого в пул-реквесте.
Однако порой бывает, что вы не уверены в каких-то мелочах. Например, в выборе имени для метода или класса. Вот об этом можно упомянуть в пул-реквесте, и тогда кто-то из коллег может предложить имя, которое вам понравится больше.
Ревьюерам также может помочь комментирование важных частей кода или даже объяснение, почему вы сделали то, что сделали. Помимо этого, с помощью комментариев вы можете направить ревьюеров к вносимым вами изменениям.
Представляйте новые идеи с помощью пул-реквестов
Как я уже сказал, лучше обсудить важные вещи до того как начать писать код. Но иногда вы можете оказаться в ситуации, когда без кода перед глазами сложно понять, будет ли работать предлагаемое решение. В подобных случаях можно создать решение – доказательство концепции, а затем сделать для него пул-реквест. Само решение не обязательно мержить, просто страница для PR-ревью станет хорошим местом для обсуждения этого решения.
Как лучше делать ревью пул-реквестов
Не одобряйте PR вслепую
Это один из худших подходов. Если большой процент разработчиков одобряют пул-реквесты, не читая, это означает, что вы на самом деле не делаете ни ревью кода, ни пул-реквесты. Вы лишь создаете видимость.
В самом начале статьи я сказал, что процедура пул-реквеста дает вам уверенность в том, что ваш код достаточно хорош для слияния. Эта уверенность происходит из знания, что другие разработчики проверили ваш код на соответствие требованиям и читаемость. Если пул-реквест достойного размера был одобрен через пять секунд после создания, – у вас проблема.
Когда вы заняты своими делами, лучше оставьте просмотр пул-реквеста другим коллегам и вернитесь к нему позже. Я предпочту подождать, пока мой пул-реквест одобрят, если при этом смогу быть уверенным, что его кто-то проверил.
Будьте вежливы и сначала спрашивайте
Порой вы не знаете, почему какой-то код был написан именно таким образом. В таких случаях бывает полезно сначала поспрашивать, а уж потом предлагать улучшения.
Если вы хорошо знакомы с кодом, предлагайте улучшения сразу. Но и в этом случае не стоит забывать о вежливости. Одну и ту же информацию можно подать по-разному. Сравните:
- «Разбей этот метод на три метода поменьше».
- «Если ты разобьешь этот метод на три метода поменьше, это повысит читабельность кода».
Не следует забывать, что люди, которые будут читать ваши комментарии, не увидят вашего выражения лица. У вас могут быть самые лучшие намерения, но, представленные в текстовом виде, они могут показаться грубостью. У меня было много ситуаций, когда я просто хотел выразиться коротко и понятно, но человек, получивший мои предложения, был этому вовсе не рад.
Используйте отрывки кода
Когда я начинал делать ревью кода, я часто писал что-то вроде «Это можно упростить», «Здесь можно использовать библиотеку lodash» и т. п. Но автор кода, получавший такие советы, не был уверен в том, что я имел в виду, и не знал, как это применить на практике.
Со временем я заменил сообщения общего характера на отрывки кода. Это занимает больше времени, но вместе с тем позволяет с легкостью сравнить существующий код с предлагаемыми улучшениями и увидеть, стоит ли их применять. Прилагаемый сниппет может не быть на 100% аккуратным, его цель – более понятно преподнести информацию автору кода.
Конечно, это лишь совет, а не жесткое правило. Если вы чувствуете, что чей-то код тяжело читать, вы можете передать свои ощущения разными способами. Иногда бывает достаточно просто высказать сомнение насчет сложности кода. Это может помочь автору заметить проблему и самостоятельно исправить код.
Делайте ревью кода, даже если вы новый человек в команде
Часто новички не уверены, чьи пул-реквесты они могут ревьюить, и когда они вообще могут начинать делать PR-ревью. В каждой команде могут быть свои соглашения насчет того, когда человек может «одобрять» пул-реквесты, но начинать читать и комментировать их нужно сразу. Вы можете даже задавать общие вопросы автору насчет кода проекта и таким образом перенимать лучшие подходы.
Как улучшить процедуру подачи и одобрения пул-реквестов
Используйте инструменты статического анализа
Одна из вещей, на которую ревьюеры обращают внимание, это стиль кода. Множественные пустые строки, неиспользованные переменные и функции и т. п. Пул-реквесты помогают привести к единообразию стили разных разработчиков. Это не значит, что какой-то стиль лучше другого. Просто единообразие лучше, чем разные стили в разных файлах.
Но использование пул-реквестов в целях приведения стилей в соответствие с определенным стандартом довольно проблематично. Прежде всего потому, что разработчики это всего лишь люди, они порой могут просто не заметить стилистические ошибки.
Кроме того, комментарии, касающиеся стиля, могут составить солидную часть всего ревью. Конечно, все эти замечания легко учесть и внести нужные исправления, но когда у вас десять комментариев насчет стиля и один насчет читаемости кода, этот последний и самый важный легко может затеряться.
Я не хочу сказать, что не следует отмечать стилистические несоответствия в PR. Просто если какие-то замечания встречаются слишком часто, подумайте об использовании линтера: это позволит заметить проблемные места до отправки пул-реквеста. Если робот может сделать что-то з вас (и даже лучше вас), зачем же делать это самому? Кроме того, когда на проблемы указывает робот, это не обидно.
Сделайте список вещей, обязательных для одобрения PR
Каждый разработчик сам решает, готов ли пул-реквест к слиянию. Но слишком рьяный ревьюер может избегать одобрения PR даже когда в нем нет больших проблем.
Если вы начинаете замечать такое поведение, обсудите в команде и условьтесь, что в коде важно. Используя это в качестве основы, определите, какие условия обязательно должны быть соблюдены, чтобы пул-реквест был одобрен.
Например, если важны тесты, введите правило, что исправление бага требует нового теста или исправления существующего.
Такой список обязательных вещей очень помогает новым членам команды и уменьшает количество дискуссий, когда PR не одобрен.
Как можно раньше прекращайте длинные треды дискуссий
Программисты любят свой код. Представьте, что вы весь день работали над каким-то алгоритмом и тут кто-то начинает его препарировать и говорить вам сделать что-то иначе. А если ревьюер еще и не слишком вежлив, ситуация усугубляется.
Ревьюер может быть прав, а может и ошибаться, но его замечание в любом случае может породить длинную ветку обсуждений, где вы будете обмениваться аргументами насчет того, какой подход лучше. Даже если дискуссия ведется вежливо, стоит ее прекратить, пока она не стала слишком длинной, и перевести ее в «живое» общение.
Иногда для этого можно просто подойти к столу другого разработчика и поговорить с ним, а иногда может понадобиться назначить встречу.
Геймифицируйте процесс
Если вам сложно заставить разработчиков делать и проверять пул-реквесты, попробуйте внести элемент игры.
Можно выводить информацию о пул-реквестах на офисном телеэкране. Скажем, вы могли бы показывать реквесты, которые уже некоторое время находятся в ожидании проверки.
Кроме того, можно вести статистику в стиле «Ревьюер, добавивший больше всего комментариев в этом месяце» или т. п. Это поощрит людей делать ревью и оставлять комментарии. Есть также много других креативных способов привлечь разработчиков к участию в этом процессе.
[customscript]techrocks_custom_after_post_html[/customscript]
[customscript]techrocks_custom_script[/customscript]
