Code review

Hozzászólások

Az ilyenre szeretem az Eclipse Save Actiont - beállítható, hogy megformázza a file-t a beállított stílus szerint. Ilyet például az IntelliJ IDEA nem tud. NetBeans sem tud. VS sem tud IMHO.

Nem a normálisság a lényeg - hanem az, hogy a csapatban mindenki ugyanazt a kódformázási stílust használja és ezt kényszerítsük ki.
Egyrészt jó az EditorConfig, több IDE is támogatja, le tudod benne írni a kódformázási szabályokat.
A kikényszerítésre lehet git hook is, ami eldobja a commitot, ha nem felel meg a szabályoknak. És lehet IDE-vel segíteni, hogy a git hook sose triggerelődjön.

És projektenként más és más lehet a szabály. Nyelvtől is függ, meg attól is, kikkel dolgozol együtt.
És én jobban szeretem, ha ezeket a tetszőleges (nem értelmetlen, de változó) szabályokat nem nekem, hanem a gépnek kell tudnia és alkalmaznia. Csinálja meg helyettem a gép, amit csak lehet.

> az, hogy a csapatban mindenki ugyanazt a kódformázási stílust használja és ezt kényszerítsük ki.
> Egyrészt jó az EditorConfig, több IDE is támogatja, le tudod benne írni a kódformázási szabályokat.

Erre egyébként van rendes tooling?
Az editorConfig megvan, de pl.:
- importok hány darab után *-gal felváltva
- sorhossz
- etc.

mert elég idegesítő, hogy a diff tele van ilyen szemét változtaásokkal, és akkor projektről projektre lehet vadászni, hogy kivel, miben, hol térünk el.
EditorConfigban csak issue-t találtam rá, pl itt: https://github.com/editorconfig/editorconfig/issues/247

szerk.: mármint nyilván bele lehet dobni a repoba az ideaból exportált dolgokat, de erre lehetne erre kellemesebb tooling is, nem?

Minden népszerű nyelvhez léteznek linterek (pl. Python). A fent említett

git hook

-os megoldás pont ezek automatikus futtatásáról szól egy

pre-commit-hook

-ban. Így addig nem enged kommitolni, ameddig maradéktalanul meg nem felel a forráskód az előírt szintaktikai szabályoknak. A szabályokat meg igény szerint projektenként vagy általánosan egy közös repóban lehet tárolni (mellékelve egy szkriptet is, ami beállítja a megfelelő hookokat friss klónozás esetén).

Feltételezem, nem kedveled a Python módszert kódblokkok létrehozására... Szerintem átláthatóbbá teszi a forrást, meg egyébként is hasonlóan formázom a kódot Pythonban, mint C-ben, csak kapcsos zárójelek nélkül. De ettől függetlenül lehet Pythonban is átláthatatlan kódot íni, legfeljebb a blokkokat jobban fel lehet ismerni.

Nem kedvelem, valóban, ugyanis a formázást a kód részévé tenni igencsak perverz dolog - anno a Fortran esetében a lyukkártyán még érthető volt, meg a whitespace nevű "nyelv" esetében is az (hiszen arról szól), de máshol...? Oké, valamiben különbözni akartak - ez lett belőle...
Ha egy nem python forrást kinyomtatok akárhogy, kézzel lefirkálok egy fecnire akárhogy, az értelmehető, használható marad. Python esetében meg nem igazán, kivéve, ha proporcionális fontot használok (és kellően nagy lépésekben van behúzva a kód), kézírásnál meg célszerűen négyzetrácsos lapra írom... Vagy külön jelölöm a behúzásokat.

Nem csak papíron - egyébként hosszabb programban hibát keresni, refaktoráláshoz átnézni jobban szeretek papíron, mint monitort bámulva, ugyanis jegyzetelni, belefirkálni, ötletelni sokkal egyszerűbb papíron.

Próbáltál már agyroggyant, 2 space/szint behúzással készült, néhány oldalas, képernyőnél hosszabb blokkokat tartalmazó idegen python programot szerkeszteni/javítani/bővíteni? Szemgúvadós picit...

Nekem eszembe se jutna kinyomtatva, papíron debuggolni. Egy normális kódszerkesztő akkora mértékben megkönnyíti a navigációt a forrásban, amit nehezen tudna a papíros módszer bármilyen előnnyel is ellensúlyozni. Pl. Sublime text-ben Ctrl+R-re kilistázza az összes függvény/osztály definíciót és elég az első pár kezdőbetűt leütni, hogy a megfelelőhöz odajussak. Emellett ha a kurzort bármely függvényre viszem, megmutatja, hogy hol van definiálva és kattintásra oda is ugrik. Továbbá természetesen ott van a sima kereső is, ami egyből a találatokhoz ugrik, ahogy ütöm be a keresési mintát. Kijelölve egy függvényt megmutatja, hány alkalommal szerepel, illetve az oldalsó kódnavigátoron mutatja is, hogy a forrásban hol helyezkednek el. A papíros módszer számomra nagyon időpazarlónak és archaikusnak tűnik, de elhiszem hogy bizonyos speciális esetekben lehet létjogosultsága.

A 2 szóközös behúzást átkonvertálni 4 szóközössé meg azért nem a legnehezebb informatikai problémák közé tartozik. :)

Van olyan, amikor muß... Egy "elhagyott" kód, amiről no doksi, de bele kell nyúlni, meg kell érteni, mit csinál, és azt módosítani/javítani, ilyen esetben inkább a nyugodt kerti fetrengés és papíron ötletelés jön be nekem, mint a monitorbámulás és papírra jegyzetelés.

Az elején írtam, hogy "refaktoráláshoz átnézni". Oké, sz@r kód estén ilyenkor sokminden megy a devnullba,de azt, hogy mit csinál, azt valahogy össze kell szedni.
Meg egy rosszul/sehogy dokumentált (egyébként príma) alkalmazásnál kideríteni, hogy hogyan lehet vele kommunikálni, milyen hívásokat enged meg, milyen paraméterezést vár - nekem ez még "szép és tiszta" kód esetén is sokszor jobban megy offline módon.

Ilyet még nem próbáltam, pedig mostanában napi szinten túrok pythont, és bele szoktam nézni libek belsejébe, hogy -- kolléga szavaival élve -- konzultáljak az egy igaz dokumentációval. Ennek több oka is van.

"2 space/szint behúzással készült"
ilyen python kódot még életemben nem láttam. Szerintem te se, vagy ha mégis, akkor sikerült megtalálnod a bolygó kisszámú konkrét elmeroggyanjának egyikét. De még ha találkoztam volna is ilyennel, és zavart volna az átlátásban, akkor azért ezt a hatalmas problémát nyomtatás előtt mintegy 1 percben orvosoltam volna.

"néhány oldalas, képernyőnél hosszabb blokkokat tartalmazó"
hát, ilyet is elég ritkán, és akkor nem nézegetni akartam, hanem keresni helyette mást, mert szart nem szívesen használok. Olyat még, hogy egy-egy függvény hosszabb legyen egy képernyőnél még csak csak, de olyat, hogy egy blokk, na olyat ritkán. Viszont ha ilyet kéne nézni, egy centivel nem lennék előbbre, ha kint lennének a jobbcici balcicik. Ha szerinted igen, akkor fogj egy ilyet, és baszd ki az elejéről az összes behúzást, aztán nézegesd :) (az ugye egyébként megvan, hogy a blokk kezdete feletti sorban mindig van egy kettőspont, csak a végén nincs semmi, csak egy dedent)

Ráadásul sose nézegetem kinyomtatva, elég sok mindenkihez hasonlóan. Nem baj az, ha te igen, de azért lásd be, az bizony elég niche usecase. (A hintaágyat (vagy a teraszt) adom, de arra meg ott a laptop. :) )

Szóval értem én, hogy neked személyesen fáj a whitespace mint olyan szignifikáns volta, néha engem is zavar, de ha az ellene a legnagyobb kifogás, hogy sosem látott módon szarul formázott, egyébként is trágyafos minőségű kódot papírra kinyomtatva kellemetlen benne nézelődni, akkor lehet, hogy revideálnom kellene az álláspontom.

nye.

A commit megtagadás ilyen mechanikus dolgok miatt szerintem hosszútávon károsabb, mint előnyös. Persze, egy humanoid dobhassa vissza code reviewban az adott dolgot - de ennyire szigorúan ragaszkodni egy code style-hoz nem hinném, hogy előnyös, mert hosszútávon biztosan lesz olyan, ahol jobb megszegni.
Másrészt a git hookkal több bajom is van:
- mindenkinél egyesével be kell állítani
- akkor inkább a pre-commitban formáztassunk rá a fájlokra automatán, de még mindig nye
- a hook egy shell script, nem? akkor azt lehet összereszelni hogy menjen windows/linux/osx/bsd/etc. alatt

etc.

lehet egyszer játszom majd egy ilyet - de sokkal nagyobb nyűgnek tűnik, mint a jelenlegi nyűgök.

En projekteknel jellemzoen engedem devbe (2-3 embernel meg oke) a linten amugy elhasalo commitokat, de masterbe mar nem (ott mar kotelezo a unit / code coverage tesztek is).

Egyszeruen sok esetben a produktivitas rovasara megy, ha maniakusan clean kodra torekszunk, foleg olyan projekteknel ahol tobb iteraciot kovetoen a business specet valtozva, erinti az architect oldalt is.

Btw en git hookot egyedul slacken hasznalok. Minden teszt a dockernel zajlik, deployolast megelozoen.

persze, ez idáig rendben is van.
sőt, így még talán a review alatt már nem lesz szemét a diffben, mert legrosszabb esetben a végén van egy commit, ami rendberakja ezeket.
de még mindig eléggé nyögvenyelősnek érzem ezt.

de nem lenne sokkal egyszerűbb, ha az ide a code completion alatt/után egyből úgy formázná a kódot, hogy mindenkinél egységes?

A flow alapesetben ugy mukodik, hogy ha egy adott kod mar nagy valoszinuseggel bekerul a projektbe, akkor elhagyhatja a feature branchet. Egy PR-t kovetoen lemennek a tesztek (lint / coverage meg opcionalisan), majd manualis jovahagyassal kerul csak a devbe, ahol megnezzuk hogy beolvaszta komponenskent mikent mukodik egyutt a rendszerrel. Innen mar viszonylag konnyedebben jut elesbe, de itt mar code coverage es lint szemszogbol is meg kell felelnie.

“nem lenne sokkal egyszerűbb, ha az ide a code completion alatt/után egyből úgy formázná a kódot, hogy mindenkinél egységes?“

Mi commitoljuk a lint configot tartalmazo fajlt is, igy a szabalyok mindenkinel egysegesek. Az IDE egyeb beallitasait is (vscode) igy ezaltal mindenki pl ugyanazokat a taskokat tudja futtatni, hogy debuggoljon vagy teszteljen. Code snippeteket ugy szinten. Egyeb beallitasokat mint peldaul tab vagy white space, beken hagyjuk. Azt mindenki ugy konfigolja ahogy akarja.

nem voltam elég pontos. valamilyen script, nem? mi alapján, a shebang alapján dől el milyen script?

Legjobb esetben majd írhatok pythonban saját lintert, mert a telepíts ezt, meg ezen az OS-en van, ott nincs dolgot ugyanúgy a nyakamba veszem még mindig. Függetlenül attól, hogy a script milyen nyelven lett megírva.

Sajnos az editorconfiggal nem lehet mindent lefedni. Persze van checkstyle, scalastyle, eslint, stb. amivel egy rakás formázási szabályt le lehet írni, jobb esetben az IDE automatikusan át is tudja venni a szabályokat.

Igaz ez nem (feltétlenül) codestyle checker, de nekem nagyon szimpatikus: https://github.com/danger/danger

az ertelmesebb diffje, nemcsak azt mutatja hogy A sor valtozott B sorra, hanem a soron belul is kepes megmutatni a valtozast, onnantol meg edesmind1 hogy 3 space-el odebbtoltad a tablazatot jobbra.

http://78.media.tumblr.com/tumblr_l4vzvfcN3Y1qanfqh.png

--
A vegtelen ciklus is vegeter egyszer, csak kelloen eros hardver kell hozza!

De akkor a whitespace változásokat is ki fogja emelni, vagyis a problémát nem oldja meg.

Arra meg még ki sem tértem, hogy erre nincs rendes tooling, az IDE nem fogja automatikusan karbantartani a táblázatot, az automatikus formázás el fogja rontani, a codestyle checker szólni fog érte (vagy ki kell kapcsolni a releváns szabályokat miatta).

Kevin Henney egyik előadásában beszél a formázásról is, elég jó elmondja mi a probléma a hasonló formázással.

Az a kedvencem mikor megvaltozik a coding convention, majd utana minden tobbezer soros fajl mentesekor a pull requestben/git logban megejelenik egy rakas style change ami miatt nem lehet kiolvasni, hogy mirol is szolt valojaban a legutobbi ctrl+s.

On the other hand: netbeans tenyleg egy vicc, ahogy kezeli az indentalast, tabokat, zarojeleket.

Tervezni kell az átállást, határidőkhöz igazodva, a legkisebb fájdalmat okozva -ezért írtam, hogy "jó esetben", azaz lesznek olyan körülmények, olyan fájlok, amikor/amikkel nem fog menni - ott a commit message/kommentezés lehet "erősebb", hogy pontosítsa azt, mi volt a pimpelésen felüli lényegi módosítás.

"majd utana minden tobbezer soros fajl mentesekor a pull requestben/git logban megejelenik egy rakas style change ami miatt nem lehet kiolvasni, hogy mirol is szolt valojaban a legutobbi ctrl+s."
Ez normálisabb esetben úgy van, hogy ha megváltozik a coding convention, akkor pontosan egy kommit és pull request szól arról, hogy minden addigi file-ban végigvezetik a változásokat, egyszerre.
Szóval nincs az, hogy minden többezer soros file mentésekor lesz whitespace change a repoban: pontosan egy kommitkor lesz ilyen.

Es ezzel a git blame parancs kimenetet el is kurtad. Dolgoztam olyan projekten, ahol nagyon fontos volt, hogy ki nyult _tenyleg_ utoljara az adott sorokhoz, es egy ilyen huzas elott 95% esellyel jo volt a git blame, utana 10% esellyel. Az ott munkaheteket jelentett egy honapban. (Igen, szar volt a szervezes, tudom)