Kultura code review i dobre praktyki PR

Przegląd kodu nie polega na pilnowaniu dostępu — dzięki niemu zespoły uczą się razem, przekazują sobie odpowiedzialność i utrzymują wysoką jakość. Dobra kultura przeglądania kodu wzmacnia cały zespół, a zła tworzy wąskie gardła i budzi frustrację.

1) Niech będzie niewielki (jeśli to możliwe, poniżej 400 wierszy). 2) Należy dodać jasny opis: dlaczego, co i jak przetestować. 3) Należy podlinkować zadanie. 4) W przypadku zmian w interfejsie należy dodać zrzuty ekranu lub nagrania. 5) Przed poproszeniem o przegląd należy samodzielnie przejrzeć diff.

Należy używać tych samych konwencjonalnych prefiksów co w commitach: feat: add user profile page, fix: handle 404 in fetch wrapper, refactor: extract Avatar component. Wiele zespołów generuje na ich podstawie changelogi.

Większość zespołów korzysta z szablonu PR — należy go zainstalować w .github/pull_request_template.md.

Przed poproszeniem o przegląd należy przejść przez własny diff wiersz po wierszu. Warto dodać komentarze wyjaśniające nieoczywiste decyzje. Często w ten sposób można wykryć własne błędy, zanim ktoś inny będzie musiał to zrobić.

PR zawierający 2000 wierszy rzadko doczeka się dokładnego przeglądu. Należy podzielić go na: 1) refaktoryzację (bez zmiany działania), 2) nowe działanie, 3) dopracowanie interfejsu. Każdą z tych części łatwiej przejrzeć i wycofać.

Uwagi należy formułować jako pytania, a nie polecenia: „Co Państwo sądzą o przeniesieniu tego do hooka?” brzmi lepiej niż „przenieść to”. Należy odróżniać kwestie wymagające poprawy od tych, które warto jedynie rozważyć. Warto używać prefiksów: nit:, question:, blocker:.

„To jest mylące” nie daje autorowi żadnej wskazówki. „Musiałem przeczytać to trzy razy, żeby zrozumieć wczesny return — czy możemy wydzielić guard clause?” daje mu konkretny punkt wyjścia do działania.

Warto pozytywnie komentować pomysłowe rozwiązania, dobre nazewnictwo i pomocne testy. Zachęca to do stosowania tych wzorców i łagodzi pozostałe uwagi. PR-y, które otrzymują wyłącznie krytykę, sprawiają wrażenie konfrontacyjnych.

Prettier zajmuje się formatowaniem. ESLint zajmuje się stylem. Nie należy marnować czasu na przeglądanie różnic między tabulatorami a spacjami. Jeśli jakaś reguła stylu ciągle się powtarza, należy zapisać ją w linterze.

Testy również są kodem. Należy upewnić się, że nowy kod ma testy. Trzeba sprawdzić, czy testy rzeczywiście sprawdzają właściwą rzecz — wiele testów przechodzi nawet wtedy, gdy kod jest zepsuty, ponieważ asercje dotyczą niewłaściwego elementu.

Należy odpowiedzieć na każdy komentarz — nawet za pomocą samej emotikony kciuka w górę. Warto polemizować z sugestiami, z którymi się Państwo nie zgadzają (to Państwo napisali ten kod i mogą znać dodatkowy kontekst). Rozwiązane wątki należy oznaczyć jako rozwiązane. Jeśli zakres zmian się zmieni, należy zaktualizować opis PR.

Przegląd należy wykonać w ciągu jednego dnia roboczego. W przypadku nieaktualnych PR-ów ginie kontekst — autor zajął się już czymś innym, a gałąź wymaga ponownego zbase'owania. Duże PR-y, które czekają tydzień, zawsze kończą się wyjątkowo trudnymi scaleniami.

Funkcja sugestii w GitHubie pozwala autorowi zaakceptować poprawkę jednym kliknięciem. Jest to znacznie szybsze niż opisowe „zmień ten wiersz na X”.

Należy zatwierdzić PR, gdy: kod jest poprawny, testy przechodzą, zmiana jest zrozumiała i można ją bezpiecznie scalić. Zatwierdzenie oznacza współodpowiedzialność za rezultat. Nie należy zatwierdzać bez czytania — jeśli kod nie został przeczytany, trzeba to powiedzieć.

Jakie nastawienie należy przyjąć, przekazując uwagi podczas przeglądu kodu dotyczące czegoś, co napisaliby Państwo inaczej?

Autor: niewielkie, dobrze opisane PR-y ze zrzutami ekranu i testami. Najpierw samodzielny przegląd. Reviewer: konstruktywne uwagi formułowane jako pytania. Należy odróżniać blocker od nit. Warto doceniać dobre rozwiązania. Styl należy pomijać — powinny się nim zajmować narzędzia. Przegląd należy wykonać w ciągu jednego dnia. PR należy zatwierdzać tylko wtedy, gdy zmiana jest zrozumiała. Szablony PR-ów ujednolicają proces. Przeglądy to współpraca, a nie pilnowanie dostępu.