Gerrit/Code review/cs

Toto je průvodce revizí a slučováním příspěvků do úložišť kódu Wikimedie, napsaný primárně pro vývojáře "provádějící" recenze kódu.


 * Vývojáři, kteří odesílají kód ke kontrole, viz Kontrola kódu.
 * Úvod do nástroje Gerrit (komentování kódu; porovnání sad patchů) najdete ve webovém průvodci Gerritem.

Cíle

 * Poskytujte rychlé recenze, abyste se vyhnuli bitrotu a pocitům opuštěnosti. Rychlá zpětná vazba povzbuzuje vývojáře, aby nadále přispívali, a pomáhá tak rozšířit naši základnu přispěvatelů.
 * Buď milý. Přispěné záplaty jsou dárky. Recenze ovlivňují vnímání dobrovolníka o celém projektu.
 * Z dlouhodobého hlediska povzbuzujte přispěvatele kódu, aby se také stali recenzenty.



Hledání oprav ke kontrole
Je zde spousta kódu ke kontrole. Jak rozdělit úkol na lépe zvládnutelné části?

Existuje několik základních vzorů:



Podle projektu
Pokud jste správce, zvažte nastavení e-mailových upozornění na nové sady patchů ve vašich projektech (úložištích) prostřednictvím "Watched Projects" v Gerritu. Případně se můžete přidat do Gerrit Reviewer Bot, který vás automaticky přidá jako recenzenta do každé nové sady patchů.


 * Identifikujte hlavní části práce nebo sady souvisejících revizí. Dobrým způsobem, jak toho dosáhnout, může být rychlý chronologický přehled. Nebo si můžete vybrat úložiště s mnoha otevřenými sadami změn.
 * Otevřete všechny stránky changesetů jako karty v okně prohlížeče. Otevřete příslušné soubory v textovém editoru.
 * Zkontrolujte nové nebo neznámé soubory jako celek. Vyberte sadu změn s hlavní změnou v příslušném souboru a zkontrolujte ji.



Podle autora

 * Vyberte autora s (mnoha) otevřenými sadami změn, načtěte jej do Gerritu.
 * Procházejte revize chronologicky nebo postupujte po jednom tématu/úložišti.
 * Tato metoda umožňuje recenzentovi poznat jednotlivé vývojáře: Jejich dovednosti, chyby a zájmy. Práce má smysl pro pokrok a kontinuitu.

Pokud vás již někdo přidal jako potenciálního recenzenta a víte, že patch nebudete recenzovat, odeberte se ze seznamu recenzentů.



Noví přispěvatelé do našich projektů
Do své nabídky můžete přidat (některý z) níže uvedených dotazů úpravou uživatelských preferencí. "Nový přispěvatel" je definován jako osoba, která přispěla celkem pěti nebo méně sadami změn.


 * Otevřené sady změn od nových přispěvatelů byly úspěšně ověřeny robotem Jenkins a čekají na zpětnou vazbu od recenzentů
 * Zkontrolujte opravu, pokud jste obeznámeni s úložištěm, a udělejte rozhodnutí (CR±1 nebo CR±2).
 * Otevřené sady změn od nových přispěvatelů úspěšně ověřeny botem Jenkins a schváleny recenzentem (CR>0)
 * Pomozte učinit rozhodnutí (CR±1 nebo CR-2 nebo CR+2, pokud máte práva +2 pro úložiště).
 * Otevřené sady změn od nových přispěvatelů úspěšně ověřeny botem Jenkins a zamítnuty recenzentem (CR<0)
 * Zeptejte se, zda přispěvatel potřebuje pomoc a zda přispěvatel plánuje pokračovat ve zlepšování sady změn.
 * Otevřené sady změn od nových přispěvatelů nebyly úspěšně ověřeny botem Jenkins
 * Zeptejte se, zda přispěvatel potřebuje pomoc a zda přispěvatel plánuje pokračovat ve zlepšování sady změn.
 * Výsledky výše uvedených dotazů jsou všechny zahrnuty v tomto jediném dotazu: Všechny otevřené sady změn od nových přispěvatelů



Chronologické (a obráceně chronologické)

 * Začněte na nejstarší otevřené sadě změn, kontrolujte, dokud nedokončíte frontu. Alternativně začněte od poslední revize a čtěte postupně každý rozdíl, dokud nedosáhnete konce. Tento přístup je vhodný pro menší revize, ale vyžaduje neustálé přepínání mezi projekty a jejich kontexty.



Kontroly checklistu


Je to chtěné

 * Úplně první otázkou je, zda je příspěvek dobrý nápad. Pokud příspěvek není užitečný nebo neodpovídá směru a rozsahu projektu, vysvětlete a poskytněte zpětnou vazbu ohledně lepších nápadů.

Obecné

 * Přidaný kód by měl fungovat tak, jak je inzerováno, to znamená, že by měly být opraveny všechny chyby nalezené v kódu. (Ale buďte opatrní, abyste neobvinili současného vývojáře z kódu napsaného předchozím vývojářem.)
 * Zachovejte zpětnou kompatibilitu pro stabilní rozhraní, pokud je to relativně jednoduché.
 * Pokud je nutná zásadní změna za účelem provedení významných vylepšení, ujistěte se, že jsou dodržovány zásady stabilního rozhraní.
 * Přečtěte si příslušné zprávy o chybách nebo dokumentaci.
 * Seznamte se se všemi relevantními technickými problémy. Přečtěte si příslušné specifikace nebo části návodu.

Výkon

 * Kód, který je v požadavku spuštěn mnohokrát, nebo kód, který se spouští při spuštění, by měl být zkontrolován z hlediska výkonu (např. členem Wikimedia Performance Team). Podezřelý kód může být potřeba otestovat.
 * Jakýkoli webový kód, který je velmi neefektivní (z hlediska času, paměti, počtu dotazů atd.), by měl být označen pro opravu (např. vytvořením úkolu pro Phabricator).
 * Změny schématu databáze nebo změny dotazů s vysokým provozem by měl zkontrolovat expert na databáze. (Odpovídající úkol Phabricator by měl mít přiřazenou značku značku "schema-change".)

Vývoj

 * Zlepší nebo zhorší tato změna uživatelský dojem pro koncové uživatele? Pokud to má vliv na uživatelskou zkušenost nebo vizuální design, zvažte konzultaci nebo designový mailing list nebo jednoho ze správců designu.

Styl

 * Ujistěte se, že jsou dodrženy konvence stylu kódování, jako jsou mezery, délka řádku, umístění složených závorek atd.

Čitelnost

 * Funkce by měly dělat to, co říkají jejich názvy. Výběr správného slovesa je zásadní, funkce get* by měla něco získat, funkce set* by měla něco nastavit.
 * Názvy proměnných by měly používat angličtinu. Pokud je to možné, nepoužívejte zkratky.
 * Preferují se komentáře k funkcím.
 * Příliš komplikovaný kód by měl být zjednodušen. To obvykle znamená prodloužen. Například:
 * Ternární operátory (?:) bude možná nutné nahradit if/else.
 * Dlouhé výrazy může být nutné rozdělit do několika příkazů.
 * Chytré použití přednosti operátorů, vyhodnocení zkratek, výrazy přiřazení atd. může být nutné přepsat.
 * Je třeba se vyhnout duplicitě, ať už v rámci souborů nebo mezi soubory.
 * Je to špatné pro čitelnost, protože je nutné hrát "zjistit rozdíl", abyste zjistili, co se změnilo. Čtení mnoha téměř kopií nějakého kódu nutně trvá déle než čtení jediné abstrakce.
 * Je to špatné pro udržovatelnost, protože pokud je v zkopírovaném kódu nalezena chyba (nebo chybějící funkce), všechny její výskyty musí být opraveny.
 * Někteří noví vývojáři mohou zkopírovat velké části kódu z jiných rozšíření nebo z jádra a změnit v něm některé drobné detaily. Pokud se zdá, že vývojář píše kód, který je pro jeho úroveň zkušeností "příliš náročný", zkuste grepovat základnu kódu pro některé fragmenty kódu, abyste identifikovali zdroj. Veďte vývojáře k přepsání nebo refaktorizaci.
 * Používání zkratek může být kontraproduktivní, protože množství času stráveného vymýšlením zkratky a ověřováním, zda funguje, bylo možné strávit úplným zapsáním původní myšlenky.

Zabezpečení

 * Recenzent by si měl přečíst a porozumět bezpečnostnímu průvodci a měl by si být vědom bezpečnostních problémů, které jsou zde probírány.
 * Neměla by existovat nejvzdálenější možnost spuštění libovolného kódu na straně serveru. To znamená, že na preg_replace by nemělo být žádné eval nebo create_function a žádný modifikátor /e.
 * Zkontrolujte problémy s vkládáním textového protokolu (XSS, SQL injection atd.) Trvejte na výstupu na výstupní straně.
 * Zkontrolujte všechny akce zápisu pro CSRF (falšování požadavků napříč weby).
 * Dejte si pozor na speciální vstupní body, které mohou obcházet bezpečnostní ustanovení v WebStart.php.
 * Dejte si pozor na zbytečnou duplikaci základních funkcí MW důležitých pro zabezpečení, jako je použití $_REQUEST místo $wgRequest nebo escapování SQL pomocí addlashes místo $dbr->addQuotes.
 * Pouze pokud pracujete na prastarém kódu: Zkontrolujte problémy s register_globals, zejména klasické svévolné chyby zabezpečení. (Register Globals byl odstraněn v PHP 5.4.0 a MediaWiki ≥1.27 vyžaduje PHP ≥5.5.9.)
 * V případě pochybností zvažte kontaktování Wikimedia Security Team.

Architektura

 * Jména, která jsou ve sdíleném (globálním) jmenném prostoru, by měla mít předponu (nebo jinak specifickou pro dané rozšíření), aby se předešlo konfliktům mezi rozšířeními. To zahrnuje:
 * Globální proměnné
 * Globální funkce
 * Jména tříd
 * Jména zpráv
 * Jména tabulky
 * Cílem modularity je oddělit zájmy. Moduly by na sobě neměly záviset neočekávaným nebo složitým způsobem. Rozhraní mezi moduly by měla být co nejjednodušší, aniž by se uchylovalo k duplikaci kódu.
 * Zkontrolujte podle Principů architektury.

Logika

 * Ukažte na zkratky a požádejte vývojáře, aby odvedl správnou práci.



Dokončení recenzi


Poskytování pozitivní zpětné vazby

 * Pokud chcete pomoci s revizí kódu, ale necítíte se pohodově (zatím) s konečným rozhodnutím, můžete použít  v Gerrit a uvést, zda jste kód "ověřili" a nebo "zkontrolovali".
 * Pokud je revize dobrá a prošla všemi výše uvedenými testy, označte ji v Gerritu . Pokud vás něčí práce obzvlášť zaujala, řekněte to v komentáři. Když označíte závazek , říkáte:
 * Zkontroloval jsem tento kód a
 * kód dává smysl a
 * kód funguje a dělá něco, co dělat chceme a
 * kód nedělá nic, co nechceme, aby dělal, a
 * kód se řídí našimi pokyny pro vývoj a
 * kód bude dávat smysl i za pět let.



Negativní zpětná vazba

 * Pokud je revize triviální, nefunkční a nemá žádnou zjevnou hodnotu, označte odevzdání jako "Code-Review -2"
 * Pokud má revize problémy, ale také má nějakou užitečnost nebo vykazuje známky toho, že směřuje správným směrem, označte ji  a přidejte komentář vysvětlující, co je špatně a jak to opravit.  Nikdy neoznačujte něco   bez přidání komentáře. Pokud někdo označí váš kód , znamená to, že váš kód je dobrý, ale potřebuje vylepšení.

Musíte zvážit náklady a přínosy každého postupu. Pokud změnu zcela odmítnete, tato změna bude ztracena a vývojáře to může odradit. Pokud chybu tolerujete, utrpí konečný produkt. Pokud to opravíte sami, necháte se vyvést z míry a možná nutíte vývojáře věřit, že je přijatelné odeslat nekvalitní kód a pak nechat někoho jiného, aby se staral o podrobnosti.

Obecné pokyny pro styl komentářů, zejména při poskytování negativní zpětné vazby:
 * 1)  Zaměřte své komentáře na kodex a jakékoli objektivně pozorované chování, nikoli na motivaci. Například neuvádějte ani nenaznačujte domněnky o motivačních faktorech, jako je to, zda byl vývojář příliš líný nebo nezkušený, aby dělal věci správně. Místo požadavků pokládejte otázky, abyste podpořili technickou diskusi: "Co si myslíte o...?" "Uvažoval jsi...?" "Můžeš to vysvětlit...?"
 * 2)  Buďte empatičtí a laskaví. Uvědomte si, že si vývojář dal se svým nápadem pravděpodobně hodně práce, a poděkujte mu za jeho příspěvek, pokud to cítíte dobře a upřímně. "Proč jsi prostě..." poskytuje úsudek a staví lidi do defenzívy. Buďte pozitivní.
 * 3)  Dejte jim vědět, kde se mohou proti vašemu rozhodnutí odvolat. Pozvěte je například k diskusi o problému na wikitech-l nebo na IRC.
 * 4)  Buďte jasní.  Necukrujte věci natolik, aby bylo hlavní poselství zastřeno.
 * 5)  A co je nejdůležitější, poskytněte zpětnou vazbu rychle. Nenechávejte negativní zpětnou vazbu jen na někoho jiného nebo nedoufejte, že nebude dostatečně vytrvalý, aby byl jeho příspěvek přijat.

