Створення pull request’у

The Repo With A Thousand Faces

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

1. Навіщо?
2. Створення pull request’у
3. Думки про pull request assignee
4. Code review & response

Англійською: On Code Reviews

При написанні pull request’у (PR), корисно керуватися принципом «вклади на оформлення стільки ж часу та/або зусиль, скільки хочеш щоб витратили на ревʼю»

Якщо автор не вкладе час на те, щоб зібрати усе до купи, тоді ревʼювери матимуть це зробити у голові. Навіть якщо ревʼювери знають про особливість у коді, вони можуть про неї не згадати підчас читання PR, тому що голова зайнята мережею звʼязків, які автор вирішив не полірувати

Якщо автор проґавить у описі PR момент, який змусив його обрати менше зло, ревʼювер так саме проґавить цей момент підчас написання коментарів

Github Stories

Як я писав минулого разу, PR’и створюються для отримання досвіду та/або думки зі сторони. Для того щоб колега могла скласти цю саму думку, для початку вона має зрозуміти, що ж відбувається у коді. Знаючи це, можна зробити правильний або неправильний висновок:

• неправильний висновок: «якщо колеги розуміють — пощастило, якщо ні — це повністю їх проблема»

• неправильний висновок: «я можу зробити PR менше або більше зрозумілим»

Для того, щоб колезі було легше сприймати PR і ділитися доречним досвідом, їй треба знати «як було раніше», «що змінюється», «чому саме це має змінитися», «як змінюється» и «чим стає». Зміни безпосередньо коду відповідають на деякі з цих питань

Наприклад, ліва/червона частина diff’у відповідає на «що змінюється», а права/зелена — на «чим стає». З рештою питань треба занурюватися у прозу

Якщо ця проза — це сухий набір фактів, то ревʼювери матимуть самостійно їх звʼязувати. Щоб спростити це, автор PR може обʼєднати їх у історію про транcформацію, у якій головний герой — це репозиторій, до якого відкрито PR

Ми розуміємо і витягуємо «суть» з історій, тому що роками чуємо/читаємо/дивимося їх. Плюс, наш мозок сприймає код як головоломку, тож ми можемо помилково ігнорувати наслідки змін — головоломку ж вирішено, кінець, з нею більше нічого не станеться… Тому має сенс розповідати історії і підчас написання PR. І у тебе є багато можливостей для цього¹

Words Mean Things

Оповідання можна почати задовго до натискання «New pull request» — у самому коді. Навіть без коментарів, ти можеш описати «основних діючих осіб» у іменах файлів, функцій та змінних. Гарне імʼя сутності опише, що і навіщо вона, покаже звʼязок з іншим кодом, і допоможе з її обговоренням. Тому важливо уникати загальних назв (на кшталт utils/data.js) та спільних імен для позначення різних речей, і звертати увагу на існуючі імена — якщо фіча вже називається wunderwaffle, то не починай раптово називати її terrifictiramisu. Звичайно, якщо зрозуміла, що файлу більше підходить інша назва, то, хоча ґіт і розумний, зазвичай краще явно зробити git mv A B — так зберігається історія і простіше створювати коміти

Імʼя гілки допоможе сфокусувати PR і не відволікати себе та ревʼюверів на другорядні «сюжети». Таким чином, у гілці JD-48/speed-up-uploads, можна спіймати і зупинити себе від оновлення усіх залежностей у проєкті JD або від змін у завантаженні файлі у проєкті ER

The Medium is the (Commit) Message

Якщо ж тебе все-таки занесло і у гілці вже купа змін, які не повʼязані з основною метою PR’у, але вони будуть корисні для проєкту та/або іншого проєкта, то можна погратися зі змістом комітів. Замість коміту з усіма змінами у файлі, нічого не заважає додати ті зміни рядково. Поціновувачі CLI можуть зробити це з git add --patch або git add --interactive, я же звик до інтерфейсу Tower’у для цього

Як зрозуміти, які рядки доречні у одному коміті, а які — у іншому? Подумай, де ці рядки опинилися би у «сюжеті» PR’у, і опиши це у імені коміту, обмежившись 50 символами, які рекомендує git². Можливо, ця історія — короткий анекдот, і 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’у у робочому чаті і видихнути, поки колеги читають, вивчають, та коментують

Перегляд PR’у заслуговує окремого запису, до якого я повернусь після того, як поділюсь парою думок про assignee