Процес розгляду запиту на вилучення

Примітка

Ця сторінка призначена для ознайомлення з процесом перегляду запитів на отримання (PR), до якого ми прагнемо. Таким чином, він в першу чергу призначений для супроводжувачів движка, які відповідають за перегляд і затвердження запитів на отримання. З огляду на це, більша частина вмісту корисна для потенційних учасників, які хочуть знати, як переконатися, що їхні PR об’єднані.

З високого рівня ідеальний життєвий цикл запиту на отримання виглядає так:

Учасник відкриває PR, який вирішує конкретну проблему (оптимально закриваючи «проблему» GitHub або реалізовуючи «пропозицію).

Інші учасники надають відгуки про PR (включно з переглядом та/або затвердженням PR, якщо необхідно).

Спеціаліст із супроводу механізму переглядає код і надає відгук, запитує зміни або схвалює запит на отримання, якщо це доречно.

Інший супроводжувач переглядає код, зосереджуючись на стилі/ясності коду, і затверджує його, коли він задоволений.

Керівник групи або член виробничої групи об’єднує запит на отримання, якщо переконаний, що він був достатньо перевірений.

Цей документ пояснює кроки 2, 3, 4 і 5 більш детально. Для більш детального пояснення робочого циклу запиту на отримання дивіться pull request workflow document.

Примітка

На практиці ці кроки можуть поєднуватися. Часто супроводжувачі надають коментарі щодо стилю коду та якості коду водночас і схвалюють запит на отримання для обох.

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

Супроводжувачі двигуна — це люди, які є «членами» сховища проектів Godot на GitHub і/або перераховані на Сторінці команд на веб-сайті Godot. Обслуговувачі відповідають за певну ділянку двигуна. Зазвичай це означає, що їм надають більше довіри щодо схвалення та рекомендації запитів на злиття.

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

Рецензування та тестування коду

Нижче наведено список речей, які учасники та супроводжувачі механізму можуть зробити, щоб провести ретельний аналіз коду запиту на отримання.

Примітка

Якщо ви хочете провести перевірку коду, але не можете зробити все в цьому списку, вкажіть це в коментарі до огляду. Наприклад, все ще дуже корисно надавати коментарі до коду, навіть якщо ви не можете створити запит на отримання локально, щоб перевірити запит на отримання (або навпаки). Не соромтеся переглянути код, тільки не забудьте зробити примітку в кінці перегляду, що ви переглядали лише код, а не тестували зміни локально.

1. Переконайтеся, що проблема існує

PR мають вирішувати проблеми, а проблеми потрібно документувати. Переконайтеся, що запит на отримання посилання та закриває (або принаймні вирішує) помилку чи пропозицію. Якщо ні, спробуйте попросити учасника оновити початкове повідомлення PR, щоб більш детально пояснити проблему, яку PR має на меті вирішити.

Примітка

Має бути зрозуміло, _чому_ потрібен запит на вилучення, перш ніж його об’єднати. Це допомагає рецензентам визначити, чи PR робить те, що він каже, і це допомагає учасникам у майбутньому зрозуміти, чому код такий, яким він є.

2. Перевірте PR і знайдіть регресії

Хоча сувора перевірка коду та CI допомагають гарантувати, що всі запити на вилучення працюють належним чином, трапляються помилки, і іноді учасники не тільки вирішують проблему, але й надсилають код, який створює проблему. Супроводжувачі уникатимуть злиття коду, який містить регресію, навіть якщо він вирішує проблему належним чином.

Переглядаючи запит на отримання, переконайтеся, що PR робить те, що він каже (тобто виправляє пов’язану помилку або реалізує нову функцію), і що зміна не порушує нічого за межами цільової області PR. Ви можете зробити це, запустивши редактор і спробувавши деякі звичайні функції редактора (додавання об’єктів до сцени, запуск GDScript, відкриття та закриття меню тощо). Крім того, переглядаючи код, шукайте підозрілі зміни в інших частинах движка. Іноді під час перебазування проскакують зміни, про які учасники не знають.

3. Перегляньте код

Перегляд коду зазвичай проводять люди, які вже мають досвід у певній сфері. Вони можуть надати ідеї, як зробити код швидшим, упорядкованішим або більш ідіоматичним. Але, навіть якщо ви не дуже досвідчені, ви можете провести перевірку коду, щоб надати відгук у межах того, що вам зручно переглядати. Це є цінним для супроводжуючого області (оскільки другий набір поглядів на проблему завжди корисний), і це також корисно для вас, оскільки це допоможе вам краще ознайомитися з цією областю коду та познайомить вас з тим, як інші люди вирішувати проблеми. Насправді перегляд коду досвідчених супроводжувачів двигуна є чудовим способом пізнати кодову базу.

Ось деякі речі, про які варто подумати та звернути увагу під час перегляду коду:

Код стосується лише областей, оголошених у PR (і повідомленні про фіксацію).

Може виникнути спокуса виправити випадкові речі в коді, як ви їх бачите. Однак це може швидко ускладнити перегляд запиту на отримання та може ускладнити пошук історії комітів. Невеликі підправки поруч із пов’язаною областю — це добре, але часто помилки, які ви можете знайти по дорозі, краще виправляти у своїх власних PR.
Код належним чином використовує власні API та шаблони Godot.

Узгодженість дуже важлива, і рішення, яке вже існує в кодовій базі, краще, ніж спеціальне рішення.
Чи торкнуться зміни основні сфери?

Іноді PR, який має вирішити локальну проблему, може мати далекосяжний ефект, виходячи за межі його сфери. Зазвичай найкраще зберігати зміни коду локально там, де виникає проблема. Якщо ви вважаєте, що рішення потребує змін поза межами проблеми, зазвичай краще запитати думку керівника групи, який може мати іншу ідею щодо вирішення проблеми.

4. Повторюйте разом із учасником і покращуйте PR

Супроводжувачі повинні надавати відгуки та пропозиції щодо покращення, якщо вони помічають у коді речі, які вони хотіли б змінити. Бажано, щоб пропозиції були в порядку важливості: спочатку розглянемо загальний дизайн коду та підхід до вирішення проблеми, потім переконайтеся, що код відповідає найкращим практикам двигуна, і, нарешті, виконайте code style review.

Примітка

Повідомте про перешкоди для об’єднання на ранніх стадіях процесу перевірки.

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

Переглядаючи запити на отримання, пам’ятайте про Кодекс поведінки Godot. Особливо наступне:

Ввічливість очікується завжди. Будь добрим і ввічливим.
Завжди припускайте позитивні наміри інших.
Зворотній зв’язок завжди вітається, але нехай ваша критика буде конструктивною.

Ось кілька речей, яких слід уникати, коли ви повторюєте запит на витягування з учасником:

Непотрібні подвійні відгуки.

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

Якість коду може бути гнучкою залежно від області механізму, у якому ви працюєте. Загалом, наші стандарти якості коду набагато вищі в основних областях і в областях, чутливих до продуктивності, ніж, наприклад, у коді редактора.
Розширення обсягу витягнутого запиту.

Надання контексту або пов’язаних/схожих проблем чи пропозицій, які можна виправити подібним чином, може бути корисним, але додавання «можна також виправити цю річ, а поки це» або «чи можемо ми додати до цього також?» не завжди чесно щодо автора. Вирішуйте, чи входять додаткові виправлення в область дії, на свій розсуд, але намагайтеся, щоб область дії була якомога ближчою до оригінального запиту на отримання.

І, зрештою, не відчувайте тиску, щоб мати справу з PR наодинці. Не соромтеся просити про допомогу в чаті Godot Contributors Chat у відповідному каналі або в #general. Можливо, інші команди вже позначені для перевірки, тому ви також можете зачекати або попросити їхньої допомоги.

5. Підтвердьте запит на отримання

Після перегляду коду, якщо ви вважаєте, що код готовий для об’єднання в механізм, тоді продовжуйте та «затверджуйте» його. Обов’язково також прокоментуйте та вкажіть характер вашої перевірки (тобто скажіть, чи запускали ви код локально, чи переглядали ви стиль, а також правильність тощо). Навіть якщо ви не є супроводжувачем движка, схвалення запиту на витягування сигналізує іншим, що код хороший і, ймовірно, вирішує проблему, як каже PR. Схвалення запиту на витягування як супроводжуючого не механізму не гарантує, що код буде об’єднано, інші люди все одно переглядатимуть його, тому не соромтеся.

Огляд стилю коду

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

З огляду на це, не всі відчувають себе достатньо впевнено, щоб надати огляд правильності коду, у такому випадку надання коментарів щодо стилю та ясності коду перед більш суттєвим оглядом коду є цілком доречним і більш ніж бажаним.

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

Переглядаючи стиль коду, зверніть особливу увагу на те, щоб запит на отримання відповідав Настанови щодо стилю програмного коду. Хоча clang-format і різні перевірки CI можуть виявити багато невідповідностей, вони далекі від досконалості і не можуть виявити деякі проблеми. Наприклад, ви повинні перевірити, що:

Дотримується стиль заголовка.

Ідентифікатори використовують snake_case і дотримуються наших правил іменування.

Параметри методу починаються з p_* або r_* (якщо вони використовуються для повернення значення).

Фігурні дужки використовуються належним чином, навіть для однорядкових умовних слів.

Код розміщено належним чином (рівно один порожній рядок між методами, жодних непотрібних порожніх рядків у тілах методів).

Примітка

Цей список не є повним і не має на меті бути повним. Щоб отримати повний набір правил, зверніться до пов’язаного документа посібника зі стилю. Майте на увазі, що clang-format може не вловити речі, на які ви сподіваєтеся, тому зверніть увагу та спробуйте зрозуміти, що саме він може виявити, а що ні.

Злиття тягових запитів

Загалом запити на витягування мають об’єднуватися лише членами виробничої групи або керівниками груп для запитів на витягування в їхній області двигуна. Наприклад, керівник мережевої групи може об’єднати мережевий запит на отримання, який суттєво не змінює немережеві розділи коду.

На практиці найкраще почекати, доки член робочої групи об’єднає запит на отримання, оскільки він уважно стежить за всією кодовою базою та, швидше за все, матиме краще уявлення про те, з якими іншими останніми/майбутніми змінами може конфліктувати цей запит на отримання ( або будь-яка інша причина, через яку має сенс відкласти запит на отримання). Не соромтеся залишати коментарі про те, що PR повинні бути готові до злиття.

Нижче наведено кроки, які необхідно виконати перед об’єднанням запиту на отримання. Ступінь, до якого ви дотримуєтеся цих кроків, може бути гнучким для простих/прямих запитів на отримання, але їх слід уважно підходити до складних або ризикованих запитів на отримання.

Як учасник ви можете допомогти просунути запит на отримання, виконавши деякі з цих кроків самостійно.

1. Отримайте відгуки від потрібних людей/команд

Члени виробничої групи повинні переконатися, що відповідні люди переглядають запит на витягування перед його об’єднанням. У деяких випадках для цього може знадобитися участь кількох людей. В інших випадках потрібне лише одне суттєве схвалення, перш ніж код можна буде об’єднати.

Загалом, намагайтеся не об’єднувати речі на основі одного лише відгуку, особливо якщо він ваш власний. Отримайте другу думку від іншого супроводжуючого та переконайтеся, що всі команди, на які це може вплинути, були належним чином представлені рецензентами. Наприклад, якщо запит на отримання доповнює документацію, часто корисно дозволити супроводжувачам області перевірити його на фактичну правильність, а супроводжувачам документації — на форматування, стиль і граматику.

Хорошим емпіричним правилом є те, що принаймні один експерт із предметної теми повинен схвалити запит на отримання правильності, і принаймні один інший супроводжувач має схвалити запит на отримання стилю коду. Будь-хто з цих людей може бути особою, яка об’єднує запит на отримання.

Переконайтеся, що відгуки та схвалення були залишені людьми, компетентними в цій конкретній галузі двигуна. Можливо, навіть багаторічний член організації Godot залишив відгук, не маючи відповідної експертизи.

Примітка

Простий спосіб знайти PR, які можуть бути готові до об’єднання, — фільтрувати за затвердженими PR та сортувати за нещодавно оновленими. Наприклад, у головному сховищі Godot ви можете скористатися цим посиланням.

2. Отримайте відгук від спільноти

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

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

3. Контрольний список Git

Переконайтеся, що PR надходить в одному коміті.

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

Для PR з кількома комітами переконайтеся, що ці виправлення змінено у відповідних комітах, а не просто застосовано поверх усього.
Переконайтеся, що PR не має конфліктів злиття.

Учасникам може знадобитися перебазувати свої зміни поверх відповідної гілки (наприклад, master або 3.x) і вручну виправити конфлікти злиття. Навіть якщо немає конфліктів злиття, учасникам може знадобитися перебазувати особливо старі PR, оскільки засіб перевірки конфліктів GitHub може не вловити всі конфлікти, або КІ міг змінитися з моменту його початкового запуску.
Перевірте правильність атрибуції фіксації.

Якщо учасник використовує підпис автора, якого немає в його обліковому записі GitHub, GitHub не зв’яже об’єднаний запит на отримання з його обліковим записом. Це не дозволяє їм отримати належну оцінку в історії GitHub і робить їх відображаються як нові учасники в інтерфейсі користувача GitHub навіть після кількох внесків.

Зрештою, користувач сам вирішує, чи хоче він це виправити, але він може це зробити, створивши комміт Git з тією самою електронною адресою, яку вони використовують для свого облікового запису GitHub, або додавши електронну адресу, яку він використовував для коміту Git, до свого профілю GitHub.
Перевірте наявність відповідних повідомлень фіксації.

Хоча у нас немає дуже суворого набору правил для повідомлень комітів, ми все одно вимагаємо, щоб вони були короткими, але описовими та використовували правильну англійську мову. Як супроводжувач, ви, мабуть, писали їх достатньо разів, щоб знати, як його створити, але для загального шаблону подумайте про "Виправити <проблему> в <частині кодової бази>". Щоб отримати детальнішу рекомендацію, перегляньте contributing.md у головному сховищі Godot.

4. Контрольний список GitHub

Перевірте цільову гілку PR.

Більшість розробок Godot відбувається у гілці master. Тому більшість запитів на вилучення потрібно робити проти нього. Звідти запити на вилучення можна потім перенести в інші гілки. Будьте обережні з людьми, які рекламують версію, яку вони використовують (наприклад, 3.3), і скеровуйте їх вносити зміни проти гілки вищого порядку (наприклад, 3.x). Якщо зміна не застосовується до гілки master, початковий PR можна зробити для поточної гілки обслуговування, наприклад 3.x. Це нормально, коли люди роблять кілька PR для кожної цільової гілки, особливо якщо зміни не можна легко перенести. Збір вишні також є варіантом, якщо це можливо. Використовуйте відповідні мітки, якщо PR можна вибрати (наприклад, cherrypick:3.x).

Примітка

Можна змінити цільову гілку PR, яка вже була подана, але пам’ятайте про наслідки. Оскільки його неможливо синхронізувати з push, зміна цільової гілки неминуче позначатиме весь список супроводжуючих для перегляду. Це також може зробити КІ нездатним працювати належним чином. Поштовх має допомогти в цьому, але якщо нічого іншого, порекомендуйте відкрити новий, свіжий PR.

Переконайтеся, що призначено відповідний етап.

Це зробить більш очевидним, яка версія включатиме надіслані зміни, якщо запит на отримання буде об’єднано зараз. Зауважте, що етап не є обов’язковим контрактом і не гарантує, що ця версія обов’язково включатиме PR. Якщо запит на отримання не буде об’єднано до випуску версії, етап буде переміщено (і сам PR може вимагати зміни цільової гілки).

Подібним чином, об’єднуючи PR із вищою віхою, ніж поточна версія, або віхою із символом підстановки (наприклад, «4.x»), переконайтеся, що оновлено віху до поточної версії.
Переконайтеся, що початкове повідомлення PR містить чарівні слова «Закриває #...» або «Виправляє #...».

Вони пов’язують PR і проблему, на яку посилається, і дозволяють GitHub автоматично закривати останню, коли ви об’єднуєте зміни. Зауважте, що це працює лише для PR, які націлені на гілку master. Для інших потрібно звернути увагу та закрити відповідні питання вручну. Зробіть це за допомогою коментаря "Виправлено #..." або "Вирішено #...", щоб чітко вказати дію для майбутніх учасників.
Для проблем, які закриває PR, додайте найближчу відповідну віху.

Іншими словами, якщо PR націлено на гілку master, але потім також вибрано для 3.x, наступний випуск 3.x буде відповідною віхою для закритої проблеми.

5. Об’єднайте запит на отримання

Якщо вам доцільно об’єднати запит на отримання (тобто ви є членом виробничої групи або керівником групи для цієї області), ви впевнені, що запит на отримання було достатньо перевірено, і після виконання цих кроків ви можете піти далі та об’єднати запит на отримання.