Ten artykuł jest jednym z dwóch artykułów z miniserii dotyczącej tematyki pull requestów oraz code review. W tym artykule omówię jak przygotować DOBRY pull request. W drugim artykule z tej serii możesz przeczytać artykuł z przeciwnej perspektywy — jak przygotować DOBRE code review.

Kompletność i stabilność rozwiązania

Moim zdaniem dobry pull request to taki gotowy do sprawdzenia. Zachęcam, by nie oznaczać nikogo do code review, zanim pull request nie będzie gotowy. Robienie code review kodu, który aktywnie się zmienia, jest nie tylko nieproduktywne, ale i irytujące. Kod w pull requeście oznaczonym do code review powinien:

• Kompilować/budować/transpilować się.
• Spełniać standardy określone w projekcie i być spójny z przyjętymi w projekcie normami i standardami. Moim zdaniem PR nie jest miejscem do dyskusji nad słusznością podejścia X nad Y. Jeśli widzimy zalety innego podejścia, to moim zdaniem warto zaproponować je w ramach osobnej dyskusji czy spotkania.
• Być pozbawiony tymczasowych logów czy komentarzy.
• Nie rzucać błędów związanych z formatowaniem. W zautomatyzowaniu tego kroku można wykorzystać git hooks, które wykryją ewentualne błędy.
• Mieć zestaw testów działających w sposób stabilny i powtarzalny.
• Pomyślnie przechodzić proces budowania/kompilacji/transpilacji i testowania na CI.
• Być uprzednio sprawdzonym samodzielnie. Zostawianie w pull requeście oczywistych baboli świadczy moim zdaniem o lenistwie i braku szacunku do osób robiących code review.
• Jeśli w projekcie jest np. checklista rzeczy do zrobienia przed review, to warto upewnić się, że wszystkie punkty z tej listy zostały wykonane.

Jeśli wystawiony pull request nie jest gotowy, to zachęcam do oznaczenia go np. etykietą. Na GitHubie można zamiast pull requesta stworzyć draft. Gdy draft jest gotowy, można przekształcić go w normalny pull request.

Za najważniejsze uważam zbudowanie świadomości w zespole, dlaczego przedstawione punkty są istotne. Listowanie tych zasad np. w szablonie pull requesta uważam, że nie ma większego sensu. Szybko wtedy zadziałają automatyzmy i przestaniemy to zauważać. Podobny efekt da przygotowanie dokumentu z wytycznymi. Jeśli ktoś nie czuje celu i istotności tych rekomendacji, to zapomni o nich po kilku dniach.

Kiedy niegotowy kod jest OK?

Wyjątkiem jest sytuacja, gdy chcemy przegadać z zespołem jakiś pomysł lub propozycję. Wtedy przygotowanie PoC czy nawet pseudokodu może mieć więcej sensu niż polerowanie kodu.

Eksperymentując, pisanie zestawu testów i polerowanie kodu może mijać się z celem. Takie rozwiązanie może zostać całkowicie zmienione czy wręcz wrzucone do kosza. W takim przypadku im szybciej dostarczymy coś do dyskusji, tym szybciej dostaniemy feedback.

Rozmiar

Mówiąc o rozmiarze pull requesta, im PR jest mniejszy, tym lepiej. Nie polecam jednak patrzenia bezpośrednio na liczbę zmienionych linii czy plików, a raczej na kontekst zmian. Zmiana polegająca na napisaniu sekcji dokumentacji mająca 500 LOC zmian może być znacznie łatwiejsza w sprawdzaniu niż 100 LOC zmian w trudnym do zrozumienia fragmencie kodu biznesowego czy porozrzucane losowo w różnych miejscach systemu. Idealnie, gdy PR skupia się wokół jednego kontekstu np. dodania nowej funkcji w systemie.

Jeśli zmiany w ramach jednego pull requesta robią się duże, to warto podzielić go na kilka mniejszych. Będzie miało to sens, nawet jeśli zmiany dotyczą jednego kontekstu, ale wielkość PR-a niebezpiecznie rośnie. Nie chcę tu rzucać konkretnymi rozmiarami. Dla jednego duży PR to będzie 250 LOC, dla innego 1000.

Duże pull requesty wpływają negatywnie na komfort robienia code review. Dodatkowo większy PR to potencjalnie więcej komentarzy i sugestii zmian. Z drugiej strony duży PR daje większe ryzyko przegapienia istotnych mankamentów w kodzie i zachęca do pobieżnego sprawdzania kodu.

Dokumentacja

Podstawową dokumentacją, którą moim zdaniem warto wykonać, jest changelog. W changelogu powinno być opisane nie to, co zostało wykonane, ale po co i dlaczego w taki sposób. To jaką metodę zmieniono w jakim pliku, można sprawdzić zaglądając w kod. Zdecydowanie lepiej opisać powód i kontekst danej zmiany. Polecam, by przy opisywaniu skupić się na biznesowych aspektach wprowadzonych zmian. Na przykład, jeśli naprawiamy jakiś błąd, to warto go opisać, jakie miał konsekwencje i co go powodowało. Jeśli PR dotyczy zmian w interfejsie użytkownika, warto dołączyć screencast lub zrzuty ekranu.

Jeśli wprowadzamy tzw. breaking changes, to również zachęcam, by to odpowiednio zaznaczyć. Warto opisać to w changelogu i opisać sposób migracji na nowe rozwiązanie. Można to również oznaczyć tytule commita np. "[BC]". Łatwo wtedy namierzyć taki commit przeglądając listę zmian.

Moim zdaniem koniecznie powinno wskazać się powiązane issue czy ticket. Pomaga to utrzymać spójną historię oraz daje dodatkowy kontekst przy code review i analizie historii zmian.

Jeśli dodane zmiany wymagają zmian w dokumentacji, również warto ją dostosować. Mam na myśli nie tylko dokumentację publiczną. Równie warto zwrócić uwagę na dokumentację wewnątrzprojektową. Praca z przestarzałą dokumentacją, szczególnie dla nowych osób w projekcie jest słaba.

Warto również dokumentować decyzje architektoniczne w postaci ADR-ów (Architecture Decision Record). Co prawda, dzięki Gitowi można cofnąć się do konkretnego commita i przeczytać changelog. Jednak uporządkowany zbiór ADR-ów jest wygodniejszy. Dodatkowo trzymanie ADR’ów w formie np. plików Markdown pozwala dodać do nich dodatkowe informacje, takie jak obrazki, mindmapy czy diagramy np. w MermaidJS.

Zasada skauta

Zasada skauta mówi:

Always leave the campground cleaner than you found it.

Podobnie jak w lesie, tak i w programowaniu warto zostawić miejsce naszego bytowania odrobinę czystszym, niż się zastało. Mogą to być drobne zmiany np. dodanie brakującej sekcji w dokumentacji, uproszczenie warunków w kodzie, poprawienie nazwy zmiennej, zrefaktoryzowanie drobnego fragmentu kodu itd. Nawet założenie issue na wyłapany problem jest już lepsze niż jego olanie.

Jednak ważne jest, by posprzątać obóz, a nie cały las. Jeśli Twoim zadaniem jest dodanie drobnej zmiany w „ośmiotysięczniku” będącym zmorą całego projektu, to jego refaktoryzacja przy okazji może nie być najlepszym pomysłem. Oczywiście jest to skrajny przykład. W każdym razie chodzi o to, by w sprzątaniu kodu przy okazji zachować umiar. Szczególnie uczulam na poprawki związane ze stylami w kodzie. Moim zdaniem takie zmiany warto wrzucać w dedykowanych pull requestach, a referencje do powiązanych commitów dodawać do pliku wskazanego w konfiguracji Gita pod kluczem blame.ignoreRevsFile. Dzięki temu zmiany w formatowaniu nie będą powodować błędnego podpowiadania autora kodu po użyciu git blame.

Komunikacja przy code review

Aspekt komunikacji po wystawieniu pull requesta jest równie ważny, jak sam kod. Warto nie utożsamiać się z kodem. Kod to tylko kod. O ile w trakcie wytwarzania kodu podejście programisty-rzemieślnika jest pomocne, tak przy code review warto zdystansować się nieco od efektów swojej pracy. W przeciwnym razie, jeśli nasze rozwiązanie zostanie „zaorane” podczas code review możemy to przyjąć zbyt osobiście. Zwykle reviewer nie ma złych intencji i nie warto odbierać komentarzy z review osobiście. Jednak zdecydowanie warto wyciągać z nich lekcje na przyszłość.

Jeśli mamy wątpliwości co do pozostawionych sugestii, to warto pytać. Lepiej dopytać i mieć klarowną sytuację niż zgadywać, a potem odkręcać zmiany, bo coś źle się zrozumiało. Zachęcam też, by ustosunkować się do wszystkich pozostawionych komentarzy, choćby w postaci zostawienia reakcji.

Przy code review warto być asertywnym. Nie zawsze zaproponowane zmiany są poprawne, adekwatne lub mieszczą się w ramach danego zadania. Warto każdorazowo analizować sensowność danej sugestii i dać znać o tym, co faktycznie o niej sądzimy.

Warto również ustalić w projekcie kto może oznaczać dany komentarz jako „resolved” – autor PR-a czy autor komentarza. Zwłaszcza dołączając do nowych projektów warto upewnić się, czy istnieje jakaś polityka w tym zakresie. Osobiście skłaniam się ku podejściu, że każdy powinien móc to robić. Wiem jednak, że są projekty, gdzie „resolve” komentarza jest robiony tylko przez jego autora.

Jeśli mamy wątpliwości co do fragmentów pull requesta, to można przy nich zostawić komentarz dopytujący. Przy wystawianiu pull requesta często sam zostawiam komentarze w miejscach, których nie jestem pewien lub gdzie wiem, że mogą pojawić się dodatkowe pytania.

Oznaczając osoby do code review, warto wybrać osoby zaznajomione z daną technologią czy częścią systemu, której dotyczy pull request. Wtedy szansa na jakościowe code review jest największa. Zachęcam też do oznaczania do code review najmniejszą możliwą liczbę osób. Po pierwsze, code review prowadzone równolegle przez kilka osób jest kontrproduktywne. Zgłoszone problemy mogą się duplikować. Po drugie im więcej głosów i opinii tym potencjalnie dłuższe dyskusje. Zachęcam do wypróbowania podejścia ze wskazywaniem do review możliwie najmniejszej liczby osób i dołączania kolejnych w razie problematycznych kwestii lub wątpliwości.

Zachęcam również, by reagować na pozostawiony feedback bez zbędnej zwłoki. Pomoże to przy ewentualnych dyskusjach lub gdy potrzebujemy o coś dopytać. Gdy wrócimy do komentarzy z review po kilku dniach, autor może już nie pamiętać kontekstu. Zmuszamy go wtedy do dodatkowego, zbędnego wysiłku. Szybka komunikacja pomoże też w szybszym zamykaniu pull requestów. Im dłużej pull request żyje, tym większa szansa na konflikty w kodzie i rozjazd z obecnym stanem projektu.

Podsumowanie

Mam nadzieję, że po przeczytaniu tego artykułu Twoje pull requesty będą jeszcze lepsze niż obecnie. Zachęcam też do sprawdzenia artykułu o tym, jak zrobić dobre code review. Dobra znajomość perspektywy z drugiej strony również pomoże w lepszym przygotowaniu i zarządzaniu pull requestami.

Źródła i materiały dodatkowe

• Michael Lynch – How to Make Your Code Reviewer Fall in Love with You
• Vadim Kravcenko – Healthy Code Reviews
• Adam Fałek – 33 najlepsze praktyki Code Review
• Jeff Mueller – Writing Tips for Improving Your Pull Requests
• The Software House – Code review z dwóch perspektyw – #33 Taby vs spacje
• Boy Scout rule definition
• dhwthompson – My favourite Git commit
• Chris Beams – How to Write a Git Commit Message
• Oskar Dudycz – A few words on communication
• Git blame – ignore revs
• Daniel Veihelmann – 5 Best Practices for Code Reviews
• Greg Foster – How large pull requests slow down development
• Jenna Kiyasu – 6 Best Practices to Manage Pull Request Creation and Feedback
• DevMentors – PULL REQUESTY – A na co to komu?!!
• Blockly – Writing a Good Pull Request

Zaobserwuj mnie na LinkedIn!

LinkedIn jest głównym medium, poza blogiem, gdzie publikuję swoje przemyślenia. Zaobserwuj mój profil na LinkedIn, by być na bieżąco!
Zachęcam też do zaobserwowania devszczepaniak.pl na Facebooku, gdzie oprócz informacji o nowych artykułach publikuję linki do ciekawych treści.

Udostępnij wpis

Szukasz taniego i dobrego VPS? Skorzystaj z Mikrusa i przy zakupie odbierz dodatkowy miesiąc usługi za darmo!

📖 Koniecznie sprawdź ofertę księgarni Helion oferującej tysiące książek dla programistów w dobrych cenach! Kupując z tego linku, pomagasz w rozwoju bloga.

Chcesz docenić moją pracę? Postaw mi kawę!