Middle: питання на співбесіді з теми «Pull request і code review»

Головна мета рев'ю - покращити здоров'я кодової бази, а не знайти ідеальне рішення. Корисно йти від загального до деталей: немає сенсу обговорювати назву змінної в коді, який узагалі не варто вливати.

Порядок перегляду:

1. Опис і мета - чи зрозуміло, яку проблему розв'язує PR, і чи потрібна ця зміна взагалі;
2. Дизайн - чи правильне місце для коду, чи вписується в архітектуру, чи не надто складно;
3. Тести - чи покривають нову поведінку й крайні випадки, чи падали б вони без зміни;
4. Логіка й коректність - граничні умови, null, порожні колекції, конкурентний доступ, обробка помилок;
5. Безпека й продуктивність - авторизація, валідація вводу, N+1, запити в циклі, великі вибірки без пагінації;
6. Читабельність - назви, зрозумілість, коментарі там, де «чому» неочевидне;
7. Стиль - лише те, що не перевіряє автоматика.

На що звертати особливу увагу в Laravel:

• міграції - чи безпечні на великій таблиці, чи є відкат, чи сумісні зі старою версією коду під час деплою;
• масове присвоєння і $fillable, перевірка прав (Gate, політики) у контролерах і Livewire-діях;
• черги - ідемпотентність задач, що буде при повторі;
• запити - with() для зв'язків, індекси під нові where.

Що віддати автоматиці:

• форматування - Pint;
• типові помилки й типи - PHPStan/Larastan;
• тести - CI.

Людина не повинна писати «тут пробіл» - на це є інструменти, а увага рецензента дорожча.

Чого не робити:

• вимагати переписати під свій смак, якщо рішення автора теж нормальне;
• рев'ювити диф без контексту - іноді треба відкрити весь файл чи запустити гілку локально (gh pr checkout 412);
• затягувати: швидкий відгук важливіший за ідеальний.

Докладніше в документації: Google: що шукати під час code review

Текстовий коментар не передає інтонації: те, що рецензент вважав дрібною порадою, автор може прочитати як різку вимогу. Тому важливо явно позначати вагу коментаря і писати про код, а не про людину.

Conventional Comments - домовленість про формат коментарів: мітка на початку показує, чого саме чекає рецензент.

issue: тут N+1 - для кожної вакансії окремий запит до company.
Додай ->with('company') у запит вище.

suggestion (non-blocking): можна винести умову в scope `published()`,
вона повторюється в трьох місцях.

nitpick: `$data` -> `$vacancyAttributes`, так зрозуміліше.

question: чи може тут прийти порожній масив? Якщо так - впаде на array_key_first.

praise: дуже зручно, що експорт іде потоково, - пам'ять не росте.

Мітки й значення:

Принципи доброго коментаря:

• пояснювати чому: не «зроби інакше», а «так буде N+1, бо...»;
• питати, а не стверджувати, коли не впевнені: можливо, автор знає те, чого не знаєте ви;
• «ми» і «код», а не «ти»: «тут можна спростити», а не «ти ускладнив»;
• пропонувати рішення чи напрямок, а не лише вказувати на проблему;
• хвалити вдалі рішення - це теж інформація: що варто повторювати;
• не дублювати: однакова проблема в десяти місцях - один коментар «і так само нижче».

Автору: відповідати на кожен коментар (виправлено, не згоден - ось чому, винесу в окрему задачу) і не сприймати рев'ю особисто - рецензують код, а не людину.

Докладніше в документації: Conventional Comments

Кнопка злиття pull request на GitHub має три варіанти, і вони по-різному формують історію main.

Create a merge commit (git merge --no-ff):

*   Merge pull request #412 from acme/csv-export
|\
| * add tests
| * fix typo
| * export service
|/
* previous commit

• зберігаються всі коміти гілки й сам факт злиття;
• легко відкотити весь PR одним git revert -m 1 <merge>;
• історія з «fix typo» і «wip» потрапляє в main.

Squash and merge:

* Експорт вакансій у CSV (#412)
* previous commit

• усі коміти PR стискаються в один новий коміт;
• лінійна чиста історія: один PR - один коміт, легко шукати й відкочувати;
• проміжні коміти автора губляться (лишаються на сторінці PR);
• автор після злиття не може просто продовжити роботу в тій самій гілці - коміт у main має інший SHA, і наступний PR покаже старі зміни ще раз.

Rebase and merge:

• кожен коміт PR переноситься на main окремо, без коміту злиття - лінійна історія з усіма комітами;
• GitHub завжди створює нові SHA й оновлює дані комітера, навіть якщо можна було б просто перемотати гілку, і відкидає порожні коміти;
• доречно, коли автор ретельно впорядкував коміти й кожен має сенс сам по собі.

Як обрати:

Політика команди: у налаштуваннях репозиторію можна залишити лише один дозволений спосіб, щоб історія була однорідною. Для squash варто налаштувати, що заголовок коміту береться із заголовка PR, - тоді якість історії залежить від якості заголовків PR.

Докладніше в документації: GitHub: способи злиття

CODEOWNERS - файл, що визначає людей чи команди, відповідальні за частини репозиторію. Коли PR змінює їхні файли, GitHub автоматично запитує в них рев'ю.

Де лежить: .github/CODEOWNERS, корінь репозиторію чи docs/ - GitHub шукає в такому порядку і бере перший знайдений. Файл діє для тієї гілки, в якій лежить.

# За замовчуванням - уся команда бекенду
*                         @acme/backend

# Міграції - обов'язково команда даних
/database/migrations/     @acme/data

# Платежі - конкретні люди
/app/Billing/             @olena @acme/payments

# Фронтенд
*.vue                     @acme/frontend
/resources/js/            @acme/frontend

# Захистити сам файл і конфіги CI
/.github/                 @acme/leads

Синтаксис: шаблони як у .gitignore, останній збіг має пріоритет. Тому загальне правило * іде першим, а конкретніші - нижче. Якщо для файла останнє правило не містить власників, у нього їх немає.

Вимоги: власники мають мати права на запис у репозиторій; команда має бути видимою й теж мати права на запис.

Справжня сила - разом із захистом гілки: правило Require review from Code Owners (у branch protection чи ruleset) не дає злити PR, доки його не схвалить власник змінених файлів. Без цього CODEOWNERS - лише автоматичне запрошення.

Практичні поради:

• захистити .github/: інакше будь-хто може в PR змінити CODEOWNERS чи workflow CI і обійти правила;
• команди, а не люди, де можливо: людина йде у відпустку - рев'ю блокується;
• не робити власником усього одну людину - вона стане вузьким місцем;
• GitHub показує помилки синтаксису CODEOWNERS прямо в інтерфейсі файла - варто перевіряти після змін.

Докладніше в документації: GitHub: про власників коду

Перший рецензент PR - його автор. Перегляд власного дифу на GitHub (а не в редакторі) за кілька хвилин знаходить те, на що інакше пішов би раунд рев'ю.

Чеклист перед запитом рев'ю:

• переглянути весь диф у вкладці Files changed - так, як побачить рецензент;
• прибрати dd(), dump(), console.log, закоментований код, випадкові файли;
• CI зелений, тести на нову поведінку є;
• опис оновлено: що, навіщо, як перевірити;
• залишити власні коментарі до неочевидних місць: «тут свідомо без транзакції, бо...» - це зменшує кількість запитань;
• PR не містить сторонніх змін, що потрапили «заодно».

Як оновлювати PR після коментарів:

• нові коміти, а не переписування історії під час рев'ю: рецензент бачить лише те, що змінилося з останнього перегляду. Після force push GitHub губить зв'язок частини коментарів з кодом, і доводиться переглядати все заново;
• fixup-коміти зручні, якщо історію треба буде впорядкувати перед злиттям:

git commit --fixup=a1b2c3d
# перед злиттям, коли рев'ю завершене:
git rebase -i --autosquash main

А якщо репозиторій вливає через squash - впорядковувати взагалі не потрібно;

• відповідати на кожен коментар: «виправлено в 4f2e1a0», «не згоден, бо...», «винесу в #430»;
• не закривати чужі обговорення без відповіді - рецензент має бачити, що його почули;
• повторно запросити рев'ю (Re-request review), коли все виправлено, - інакше рецензент не дізнається, що черга знову за ним.

Якщо коментарів дуже багато - часто це сигнал, що варто поговорити голосом чи переглянути підхід, а не виправляти по одному.

Докладніше в документації: GitHub: запит на рев'ю