Создание Pull Request’а

The Repo With A Thousand Faces

Серия записей про code review:

1. Зачем?
2. Создание Pull Request’а
3. Мысли про Pull Request Assignee
4. Code Review & Response

При написании Pull Request’а (PR), полезно руководствоваться принципом «потрать на оформление столько же времени и/или усилий, сколько хочешь, чтобы потратили на ревью»

Если автор не потратит время на то, чтобы собрать всё воедино, то сделать это должны будут ревьюверы в голове. И даже если ревьювер знает про проблему в коде, она может про неё не вспомнить во время чтения кода, потому что голова занята сетью связей, которые автор решил не причёсывать

Если автор упустит в описании PR момент, который заставил его выбирать меньшее зло, ревьювер точно так же упустит этот момент при написании комментариев

Github Stories

Как я писал в прошлый раз, PR’ы создаются для получения опыта и/или мнения со стороны. Чтобы коллега могла составить это самое мнение, она должна для начала понять, что же происходит в написанном тобой коде. Зная это, можно сделать неправильный или правильный вывод:

• неправильный вывод: «если коллеги понимают — повезло, если нет — то это исключительно их проблема»
• правильный вывод: «я могу сделать PR менее или более понятным»

Для того, чтобы коллеге было легче воспринять PR и поделиться подходящим опытом, она должна знать «как было раньше», «что меняется», «почему надо поменяться именно этому», «как меняется» и «чем становится». Изменения кода частично отвечают на некоторые из этих вопросов

Например, левая/красная часть diff’а отвечает на «что меняется», а правая/зелёная — на «чем становится». С остальными вопросами надо окунуться в прозу

Чтобы эта проза не была сухим набором фактов, которые нужно связать самостоятельно, автор PR может объединить их в историю о трансформации, где герой истории — это репозиторий, в который открыт PR

Мы умеем понимать и извлекать «суть» из историй, потому что годами слышим/читаем/смотрим их. Плюс, наш мозг воспринимает код как головоломку, так что мы можем ошибочно забить на последствия изменений — головоломка ж решена, конец, ничего с ней больше не произойдёт. Так что имеет смысл воспользоваться этим и при написании PR. И у тебя есть много возможностей для того, чтобы рассказать историю¹

Words Mean Things

Повествование можно начать задолго до нажатия «New pull request» — в коде. Даже без комментариев, ты можешь описать «основные действующие лица» в именах файлов, функций и переменных. Хорошее имя сущности опишет, что и зачем она, покажет на связь с другим кодом, и поможет с её обсуждением. Поэтому важно избегать общих названий (вроде utils/data.js) и общих имён для обозначения разных вещей, и обращай внимание на уже существующие имена — если фича уже называется wunderwaffle, то не начинай внезапно называть её terrifictiramisu. И да, если понял, что файлу больше подходит другое имя, то, хоть гит и умный, зачастую лучше явно сделать git mv A B — так сохраняется история и легче создавать коммиты

Имя ветки поможет сфокусировать PR и не отвлекать себя и ревьюверов на второстепенные «сюжеты». Так, в ветке JD-48/speed-up-uploads, можно словить и остановить себя от обновления всех зависимостей на проекте или от изменений в загрузке файлов другого проекта

The Medium is the (Commit) Message

Если же всё-таки тебя занесло и в ветке уже куча правок, несвязанных с основной целью PR’а, но которые полезны для проекта и/или другого тикета, то можно поиграться с содержимым коммитов. Вместо коммита со всеми правками в файле, ничего не мешает добавить их построчно. Любители CLI могут это сделать с git add --patch или git add --interactive, я же привык к интерфейсу Tower’а для этого

Как понять, какие строки уместны в одном коммите, а какие — в другом? Подумай, где эти строки оказались бы в истории PR’а, и опиши это в сообщении коммита, ограничившись рекомендуемыми git’ом 50 символами². Может, эта история — короткий анекдот и fix A when B будет достаточно. Может, в «сюжете» PR несколько персонажей, которых надо представить сначала читателю (add X to do Y), а потом друг другу (use X in Z)

Describe your changes in imperative mood, e.g. «make xyzzy do frotz» instead of «[This patch] makes xyzzy do frotz» or «[I] changed xyzzy to do frotz», as if you are giving orders to the codebase to change its behavior. Try to make sure your explanation can be understood without external resources. Instead of giving a URL to a mailing list archive, summarize the relevant points of the discussion.

— SubmittingPatches - The core git plumbing

Также, во многих проектах настроена связь между Git’ом и Jira, чтобы подвязывать коммиты и/или ветки к тикетам. Так что часто имеет смысл указывать номер тикета в имени коммита, например, #JD-48 cache responses from X. Формат (XX-00, $XX-00, #XX-00) отличается от проекта к проекту, но тому же Github’у достаточно любого префикса перед цифрами для того, чтобы отобразить их в веб-интерфейсе как ссылки на тикеты

Многие ещё следуют conventional commit и добавляют теги вроде feat и chore. Мне эти теги кажутся бесполезными для продукта без публикуемого CHANGELOG’а, но привычка категоризовать изменения может помочь с фокусированием PR и коммитов

Retelling the Story

Если же напутал с порядком коммитов, то его можно починить вызовом git rebase --interactive, с которым легко переставить/переписать/объединить/вообще выбросить коммиты, редактируя текстовый файл со списком операций и коммитов

Например, мы работали в ветке JD-48 и написали пять коммитов. Локальная история коммитов теперь выглядит как-то так (git log --graph --oneline --all, новые коммиты сверху) :

* f4593f9 (HEAD -> JD-48) add tests for X-Y integration
* 04d0fda optimize Y
* ba46169 integrate X with Y
* fa1afe1 tests for X
* 5928aea setup X
* 300a500 (master, origin/master, origin/HEAD) …

git rebase --interactive "5928aea^" откроет в текстовом редакторе³ файл с такими строчками:

pick 5928aea setup X
pick fa1afe1 tests for X
pick ba46169 integrate X with Y
pick 04d0fda optimize Y
pick f4593f9 add tests for X-Y integration

Каждая строчка — это операция, которую нужно совершить при создании новой версии истории. По умолчанию, rebase берёт каждый коммит и применяет его (pick) . Кроме этого, коммиты можно пропускать (drop), объединять с предыдущим (squash), применять-с-возможностью-редактирования (fixup), можно менять их порядок перестановкой строк, можно запускать произвольные shell-команды (exec)

Например, если мы хотим:

• перенести коммит с тестами (fa1afe1) к концу ветки;
• объединить его с тестом интеграции (f4593f9);
• и выбросить коммит про оптимизацию другой части проекта (потому что PR про новую фичу, а не про оптимизацию)

то нужно отредактировать «сценарий», который сгенерировал git rebase --interactive, в такой:

pick 5928aea setup X
pick ba46169 integrate X with Y
drop 04d0fda optimize Y
pick fa1afe1 tests for X
squash f4593f9 add tests for X-Y integration

Либо можно не выделываться и переписывать историю мышкой в GUI, вроде уже упомянутого Tower’а

После того, как ветка и коммиты причёсаны, можно взяться за имя и описание PR. Они, по большому счёту, пишутся аналогично коммитам, но намного более высокоуровнево

Что писать в описании Pull Request’а?

Зачастую, самое сложное в описании PR — это начать. Чистый лист <textarea> многих блокирует

В некоторых репозиториях есть PULL_REQUEST_TEMPLATE.md, который может помочь с этим, но в приватных проектах его часто либо нет, либо он довольно бесполезный для предоставления контекста:

# Summary
...

---

- [ ] <!-- some-continuous-integration-checkbox --> ci-bot, do something

С пустым <textarea> также могут помочь сообщения коммитов. Если же их недостаточно, то начни с «Короче, есть существительное и оно глагол» и продолжай обвешивать это вступление контекстом

Этим контекстом могут служить:

• причины для изменений (не «мне менеджер сказала», а «у пользователей проблема»)
• странные места, где кажется что можно лучше, но лень или не знаешь как. Может они не проблемные, может кто-то знает хорошее решение, может кто-то смотивирует побороть лень
• ссылки на связанные тикеты и PR; на документацию новых/странных моментов; на ответы с StackOverflow, с которых ты скопировал тот сниппет…

Кстати про ссылки. Десяти минут на изучение Markdown’а ⁴, помогут с тем, чтобы эти ссылки а) не мешали читать и писать описание; б) находились в подходящем месте

Например, сравни читабельность текста с разными стилями ссылок:

There’s [`<input type="color">`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/color),
but we [can’t use it](https://caniuse.com/#feat=input-color) yet in all of
[our supported browsers](https://kb.example.com/supported-browsers)

There’s `<input type="color">`, but we can’t use it yet in all of our supported browsers

Links:
- https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/color
- https://caniuse.com/#feat=input-color
- https://kb.example.com/supported-browsers

There’s [`<input type="color">`][mdn], but we [can’t use it][caniuse] yet in all of [our supported browsers][kb]

[mdn]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/color
[caniuse]: https://caniuse.com/#feat=input-color
[kb]: https://kb.example.com/supported-browsers

Передача эстафеты

После того, как вся проза написана, ей нужна аудитория. Назначь в assignee заинтересованных коллег (будь то отдельные юзернеймы, целые команды, или только ответственные за ревью) и, закончив с текстом, который так или иначе отображается на Github’е, можно потратить минуту времени на «анонс» Pull Request’а в Slack’е, и выдохнуть, пока коллеги читают, изучают и комментируют PR

Просмотр PR заслуживает отдельной записи, к которой вернусь в следующий раз после того, как поделюсь парой мыслей про assignee