Reguły zamiast review

Model potrafi wygenerować trzysta linii w minutę. Przejrzenie tych trzystu linii zajmuje mi czterdzieści i połowę tego czasu spędzam, wypisując w komentarzach rzeczy które wypisałem już w zeszłym tygodniu w innym PR-ze. Odpowiedzialność za jakość kodu przesunęła się prawie w całości na drugiego dewelopera I to jest wąskie gardło, którego nie da się rozwiązać zatrudnieniem kolejnego.

TL;DR

Wszystko, o czym piszę, stoi w moim boilerplacie: symfony-boilerplate-ddd.

Co zostaje człowiekowi, a co maszynie

Review ma dwie warstwy i tylko jedna z nich potrzebuje człowieka.

Pierwsza to pytania, na które odpowiedź wymaga kontekstu spoza repozytorium: czy ten model domenowy oddaje to, o czym rozmawialiśmy z biznesem, czy ten trade-off jest wart swojej ceny, czy nazwa tej metody będzie zrozumiała za rok. To jest praca dla człowieka i długo nią pozostanie.

Druga to sprawdzanie po raz dwudziesty, czy ktoś znowu nie wstrzyknął EntityManagera do domeny. To jest praca dla maszyny i zawsze nią była. Dopóki kod pisali wyłącznie ludzie, dawało się ją utrzymać dobrą wolą i pamięcią kolegi z zespołu, który akurat patrzył na PR-a. Ten model przestał działać, kiedy objętość generowanego kodu urosła szybciej niż liczba osób, które mają go przejrzeć.

Jest jeszcze druga przyczyna, mniej oczywista. Model nie zna ustaleń, których nie ma w kontekście. Nie był na spotkaniu, na którym zdecydowaliście, że mapowanie Doctrine idzie do infrastruktury. Zna to, co dostanie w promptcie, i to, co powie mu narzędzie, kiedy uruchomi testy. Jeśli kontrakt zespołu żyje wyłącznie w głowach, model go nie pozna. Jeśli żyje jako reguła, dostanie go zwrotnie przy każdym uruchomieniu analizy i sam się poprawi, zanim PR wogóle do mnie trafi.

Warstw pilnuje Deptrac

Zacznę od rzeczy najprostszej, bo daje najwięcej za najmniejszy koszt. Deptrac przypisuje klasy do warstw, i sprawdza czy zależności między nimi idą w dozwoloną stronę.

Powstał w czasach, w których diagram warstw wisiał na Confluence, a trzymanie się go było kwestią dyscypliny. Ktoś rysował pudełka ze strzałkami, zespół zgadzał się ich pilnować, a po pół roku okazywało się, że domena importuje EntityManagera, bo tak było szybciej w piątek po południu. Zamysł był prosty: przenieść ten diagram z Confluence do repozytorium, żeby dało się go sprawdzić zamiast w niego wierzyć.

Ten zamysł się nie zmienił. Zmienił się odbiorca. Wcześniej Deptrac był narzędziem kontrolnym, odpalanym rzadko: przy większym refaktorze albo przed wydaniem, żeby zobaczyć, jak daleko odjechaliśmy od projektu. Dziś jest przede wszystkim narzędziem informacyjnym dla kogoś, kto tego diagramu nigdy nie widział. Model nie ma dostępu do Confluence'a. Ma dostęp do outputu composer test:architecture, który powie mu wprost, że App\Order\Domain\Order nie ma prawa zależeć od Doctrine\ORM\EntityManager.

To jest cały trik: jedna komenda zamienia ustalenie architektoniczne w informację zwrotną, którą da się dostać w pętli, przy każdej zmianie zamiast raz na kwartał.

layers:
  - name: Domain
    collectors:
      - type: classLike
        value: '^App\\[A-Za-z0-9]+\\Domain\\'

  - name: Application
    collectors:
      - type: classLike
        value: '^App\\[A-Za-z0-9]+\\Application\\'

ruleset:
  Domain: ~

  Application:
    - Domain

  Infrastructure:
    - Domain
    - Application
    - Vendor

Domain: ~ to pusta lista dozwolonych zależności, czyli domena nie może zależeć od niczego poza sobą. Aplikacja widzi domenę. Infrastruktura widzi wszystko, w tym vendora.

Ciekawszy jest sposób, w jaki zdefiniowana jest warstwa Vendor: nie jako lista paczek, tylko negatywnie: wszystko, co nie jest App\, z białą listą typów wbudowanych PHP (DateTimeImmutable, Throwable, Countable i podobne). Dzięki temu każda nowa zależność z Packagista jest domyślnie zakazana w domenie, bez dopisywania czegokolwiek do konfiguracji.

Odpowiedzialności wszystkich warstw, zapisane w pięćdziesięciu linijkach YAML-a i sprawdzane jedną komendą. Nie ma nic tańszego.

Własne reguły PHPStana

O poziomach PHPStana i o tym, że warto włączyć level: max, napisano już wszystko. Mniej mówi się o sekcji rules:, czyli o tym, że do analizatora można dopisać własne reguły, które nie mają nic wspólnego z typami, a wszystko z tym, jak wasz zespół umówił się pracować.

Reguła to jedna klasa z dwiema metodami. getNodeType() zawęża drzewo składniowe do interesującego węzła, processNode() decyduje, czy to błąd:

final class NoAmbientClockRule implements Rule
{
    private const DOMAIN_NAMESPACE_MARKER = "\\Domain\\";

    public function getNodeType(): string
    {
        return New_::class;
    }

    public function processNode(Node $node, Scope $scope): array
    {
        if ($this->isDomainNamespace((string)$scope->getNamespace())) {
            return [];
        }

        if (!$node->class instanceof Name || $node->class->toLowerString() !== "datetimeimmutable") {
            return [];
        }

        $argument = $node->getArgs()[0]->value ?? null;

        if ($argument === null || ($argument instanceof String_ && strtolower($argument->value) === "now")) {
            return [
                RuleErrorBuilder::message(
                    "Do not read the ambient clock — inject Psr\\Clock\\ClockInterface (symfony/clock provides it) and take the current time from its now(). (Domain classes are exempt, they may stamp themselves.)",
                )->identifier("symfonyBoilerplate.noAmbientClock")->build(),
            ];
        }

        return [];
    }
}

Trzydzieści linii, a pilnują nawyku, który normalnie wychwytuje się dopiero wtedy, gdy trzeba napisać test na coś, co zależy od „teraz". Zwróć uwagę na identifier. Wrócę do niego za chwilę bo to najbardziej niedoceniana część tego API.

Rejestracja to jedna linia w phpstan.neon:

rules:
    - SymfonyBoilerplate\PhpStan\NoAmbientClockRule

14 reguł, które trzymają kontrakt

W boilerplacie mam ich czternaście. Nie dlatego, że lubię reguły, tylko dlatego, że każda z nich zastąpiła komentarz, który pisałem więcej niż raz.

Widać w tym trzy rodzaje kontraktu. Kilka reguł pilnuje warstw, czyli tego, czego Deptrac nie złapie, bo dzieje się wewnątrz jednego namespace'u. Kilka pilnuje interfejsów: skoro klasa implementuje RequestInterface, to argument resolver ją zdeserializuje, więc musi być final i niemutowalna, a reguła zapisuje, gdzie i jak wolno ten interfejs implementować. Reszta pilnuje zwykłych nawyków, które w komentarzu do PR-a brzmią jak czepianie się.

Komunikat błędu pisz dla modelu

Skoro większość pisania kodu oddaliśmy modelom, to warto spojrzeć na komunikaty błędów z narzędzi jak na wejście, a nie wyjście, bo to one wracają do pętli. Tu właśnie najczęściej marnuje się potencjał własnych reguł. Porównaj dwa komunikaty z tego samego repozytorium.

Ten jest zły, mój własny, do poprawki:

Use DateTimeImmutable, not mutable DateTime.

A ten dobry:

Domain class names the framework type "%s" — the domain layer stays framework-free; depend on an interface you own and adapt the framework type in the Infrastructure layer.

Różnica nie polega na długości. Pierwszy mówi, czego nie wolno. Drugi mówi, co zamiast, i podaje warstwę, w której należy to zrobić. Dla człowieka to różnica między irytacją a podpowiedzią. Dla modelu to różnica między zatrzymaniem się a naprawieniem kodu w następnej iteracji, bo output analizatora jest dokładnie tym, co dostaje z powrotem do kontekstu.

Komunikat reguły to nie linijka loga, tylko jedyny kawałek dokumentacji, który napewno zostanie przeczytany. Pisz go jak akapit z onboardingu.

Drugą rzeczą jest identifier. To adres reguły, po którym wycisza się pojedyncze przypadki zamiast całej reguły:

ignoreErrors:
    -
        identifier: symfonyBoilerplate.noAmbientClock
        path: tests/*
        reportUnmatched: false

W testach czytanie zegara jest w porządku. Wyjątek jest tu jawny, opisany i ograniczony do jednej ścieżki, a nie schowany w @phpstan-ignore-next-line rozsianym po kodzie.

Wady?

Żeby było uczciwie:

• Reguła to kod, który trzeba utrzymać. Testujesz ją, poprawiasz, aktualizujesz razem z PHPStanem.
• False positives potrafią zaboleć. Reguła, która blokuje poprawny kod, w tydzień wyląduje w ignoreErrors i przestanie cokolwiek chronić.
• Regułą nie zapiszesz sensu. Ani jedna z tych czternastu nie powie ci, że model domenowy jest zły. Zwalniają czas na to pytanie, ale nie odpowiadają na nie.
• Wdrożenie w istniejącym projekcie boli. Trzeba zacząć od baseline'u i schodzić z długiem stopniowo, bo inaczej pierwszy przebieg wypluje kilkaset błędów i wszyscy to wyłączą.

Podsumowanie

Zasada jest jedna: jeśli powiedziałeś to samo w komentarzu do PR-a trzeci raz, to nie jest już uwaga do kodu, tylko brakująca reguła.

Wcześniej ten mechanizm działał na pamięci kolegi z zespołu i tak długo, jak kod powstawał w ludzkim tempie, wystarczał. Dziś nie wystarcza, i nie dlatego, że modele piszą gorzej, tylko dlatego, że piszą szybciej, niż jesteśmy w stanie czytać. Kontrakt, który zapiszesz jako regułę, egzekwuje się sam przy każdym uruchomieniu, a przy okazji wraca do modelu jako podpowiedź, zanim ktokolwiek otworzy PR-a.

Człowiekowi zostaje ta część review, w której faktycznie jest potrzebny.

Masz pytania? Napisz na LinkedIn.