Перевод статьи «How to Make a Good Pull Request».
Начинающему программисту часто страшно просить людей проверить его пул-реквест. Вашу работу нужно просмотреть, но вы не хотите никого беспокоить! Между тем, показывать свой код — часть работы разработчика и даже часть его становления как профессионала. В этой статье мы дадим несколько советов, как облегчить ревьюерам проверку ваших пул-реквестов и избавить себя от лишних хлопот.
Оставляйте строчные комментарии в самом пул-реквесте
Если из всей статьи вы возьмете на вооружение только один совет, пускай это будет именно первый пункт.
Строчные комментарии — феноменальный способ рассказать людям, что вы имели в виду, когда писали тот или иной отрывок кода. При этом они не приводят к дебатам на тему, нужен ли этот комментарий в коде. Строчные комментарии в самом пул-реквесте ускоряют ревью и не загромождают кодовую базу лишними вещами.
Рассмотрим комментарий:
«Нам пришлось изменить все эти h1 на h2 из соображений доступности. Учитывая, что формы используются только встроенные, а не в виде всплывающих модальных окон, best practice — делать заголовки подзаголовками. В любом случае, стиль — по-прежнему h3, так что при QA не должно быть никаких изменений».
Такой комментарий в коде был бы совершенно ужасным. Он слишком длинный, все это можно понять, если вчитаться в сам код, а кроме того, в нем упоминается QA. Но в качестве комментария к пул-реквесту он просто отличный. В комментарии раскрыты ответы на вопросы, которые могут возникнуть у вашего ревьюера, а также поясняется, каким будет поведение при тестировании.
Добавляйте подобные комментарии к пул-реквесту, когда:
- На проработку логики может потребоваться некоторое время, поэтому подсказка будет кстати.
- Вносимые изменения требуют знания best practices, которого у ревьюеров может и не быть.
- В пул-реквесте какая-то информация берется из других, неизмененных файлов, которые не затрагиваются в ревью (ваш комментарий избавит ревьюера от необходимости открывать еще один файл и читать его).
Все эти комментарии ускоряют работу и уменьшают количество вопросов, которые могут возникнуть у ревьюера.
Ограничьте размер пул-реквеста сотней строк кода* (или меньше)
Это совет из серии «Ага, мы все это знаем, а теперь покажи, как это сделать».
Есть шутка о том, что если вы дадите программистам 10 строк кода на проверку, они найдут 10 проблем, но если вы дадите им 1000, они скажут: «Выглядит хорошо!». Это смешно, потому что обескураживающе правдиво.
Чтобы досконально проверить несколько тысяч (или даже сотен) строк кода, потребуется несколько дней. Но если в пул-реквесте будет всего сто строк, ваш ревьюер с куда большей вероятностью сумеет изолировать вашу работу от предыдущего кода и дать качественный отзыв.
А чтобы держать размер пул-реквестов в узде, есть маленький, до действенный прием: плясать надо от тикета.
*В эту сотню строк не входят тесты, снапшоты или сгенерированные файлы.
Все, что не связано с темой вашего тикета, отправляется в новый тикет
Это очень важный момент. Добавление нескольких строк, не связанных с темой тикета, вероятно, является одной из наиболее распространенных ошибок.
У вас может возникнуть соблазн реорганизовать вещи так, как вам кажется правильным, но, когда дело дойдет до ревью, это может вызвать путаницу.
Вам действительно НУЖНО было реорганизовать эту функцию? Это было как-то связано с вашим тикетом? Или вы просто исправили функцию, раз уж все равно находились в этом файле? Помните, что вашему ревьюеру не всегда это будет ясно.
Поэтому, заметив что-то, не имеющее отношения к вашему текущему тикету, следует это внести в новый тикет и поместить в бэклог. Благодаря этому все ваши пул-реквесты будут сфокусированными, и над ними будет легче работать.
Если вдруг окажется, что ваш исходный тикет требует больше работы, чем вы думали, разбейте его на несколько тикетов или несколько PR. Смысл в том, что отдельные порции работы должны быть небольшими.
Но не стесняйтесь проводить рефакторинг и улучшать все, что имеет отношение к вашему тикету. Если сама функция, в которую вы хотите что-то добавить, нуждается в некоторой полировке, дерзайте! В результате необходимого рефакторинга ваш код будет легче читать. А вот если функция не имеет ничего общего с вашей текущей задачей, выделите эту работу по рефакторингу в отдельный тикет.
Пишите тесты как пользовательские истории
Тесты должны быть полезны для ревьюера. Если ревьюер видит тесты типа «Отображает новое модальное окно при нажатии кнопки», «Кнопка «Далее» отключается при появлении модального окна» и «Не разрешает отправку до тех пор, пока все поля не будут иметь действительные записи», ему довольно ясно, что ваш код должен делать.
Помимо дополнительных объяснений, многие тесты снимают с ревьюера часть нагрузки. Благодаря им он не чувствует себя единственным, что стоит между продакшеном и вашим кодом, ведь есть множество тестов, которые проверяют вашу функцию программными методами.
Такое распределение ответственности сделает ваши пул-реквесты менее стрессовыми, и люди с большей вероятностью будут соглашаться проверить ваш код.
Объясните, как именно тестировать ваш код
Если среду запуска нужно настроить каким-то определенным образом или входные данные должны быть особыми — сообщите об этом коллегам!
По сути, если, выполняя работу над тикетом, вам приходилось предпринимать какие-либо шаги по настройке, об этом обязательно нужно написать и вашему ревьюеру, и тестировщикам. Что-то типа:
«Чтобы протестировать новые проверки, запустите среду разработки и заполните форму до последнего раздела. Затем попробуйте добавить неверный номер телефона, адрес электронной почты или имя. Кнопка «Отправить» должна оставаться серой, а каждый неверный ввод должен получать красную окантовку. Отдельно нужно попробовать добавить числа и символы, такие как !, $, ^, в поле имени. Теперь они не должны проходить».
Критерии приемки — обязательны
Если ваша команда не имеет критериев приемки для тикетов, я настоятельно рекомендую добавить их. Это даст вашим ревьюерам конкретные примеры того, на что следует обращать внимание при запуске вашего кода. В идеале, эти ситуации также будут подвергнуты тестированию.
Если вы что-то делаете в интерфейсе, также будет полезным добавление скриншотов «до и после». Это особенно важно, если пользователь может увидеть несколько разных результатов.
Исходите из предположения, что коллеги не знают, чем вы занимаетесь
Да, это странный совет, но я сейчас разовью эту мысль. Работа над каждым тикетом связана с внезапными озарениями. Но нет никаких оснований предполагать, что у вашего будущего ревьюера тоже были эти озарения. Иногда людей просят проверить код, написанный для проектов, с которыми они вообще не слишком хорошо знакомы.
Если у вас было какое-то озарение, позволившее вам успешно завершить работу, сообщите о нем в комментарии к пул-реквесту.
Приведу пример.
В одном из моих проектов постоянно появлялись два индекса. Но, как оказалось, на самом деле индексом был только один. Второй был кодом операции, вызывавшим несколько более глубоких функций. Я добавил несколько именованных переменных, чтобы устранить предположение об индексе. Но также я добавил комментарий к PR с упоминанием функции, в которой использовался тот код операции, и того, что эта функция возвращала.
Прислушивайтесь к критике
Это, вероятно, важнее всего остального. Независимо от того, насколько хорош ваш код, никто не захочет его проверять, если вы упрямы, как осел. Спорить на тему кода — это нормально, но, получая фидбэк, всегда оставайтесь профессиональным и вежливым. Не становитесь разработчиком, для которого существует только два вида мнений: его и неправильное. Когда кто-то указывает на проблему или задает вопрос, предположите, что ваш коллега прав, и проработайте эту тему.
Не отмахивайтесь от слов коллег сразу же и не игнорируйте их. Если вы правы, докажите это примерами и ссылками. А если окажется, что вы ошиблись, — ничего страшного! Цель код-ревью — использовать команду для создания лучшего кода, а не просто использовать ваш код.
Помните, что любое ревью, в результате которого вы чему-то научились, — хорошее ревью.
[customscript]techrocks_custom_after_post_html[/customscript]
[customscript]techrocks_custom_script[/customscript]