Git Essentials: een stapsgewijze handleiding voor het beoordelingsproces van pull-requests (2023)

Git is de facto een standaard geworden voor versiebeheer van broncode en om er het maximale uit te halen, moeten we niet alleen begrijpen hoe het werkt, maar ook hoe we de belangrijkste samenwerkingsfunctie kunnen gebruiken: pull-aanvragen!

De meeste ontwikkelaars hebben van pull-aanvragen gehoord en deze geopend. Voor velen van ons betekent dit een enorme stap voorwaarts in ons codeleveringsproces. We hebben wat code geschreven, we zijn blij met het resultaat en we willen zeggen dat we klaar zijn. Maar om dat te bereiken, moeten we ons pull-verzoek goedgekeurd krijgen of misschien dat van iemand anders goedkeuren.

In deze blogpost duiken we erintips en richtlijnen voor pull request-deelnemersom het meeste uit deze Git-functie en het proces eromheen te halen. Bij Comtrade Digital Services streven we altijd naar de best mogelijke codekwaliteit en dit is een klein stukje van de puzzel dat ons helpt dat doel te bereiken. Mogelijk gebruikt u een broncodehostingservice zoals GitHub, BitBucket of GitLab, maar het beoordelingsproces is hetzelfde en de volgende tips kunnen overal worden toegepast.

Waar bevindt zich het pull-verzoek in de workflow van de ontwikkelaar?

Je kreeg een taak. “We moeten deze functie dringend ontwikkelen”, zeiden ze. “Als het gisteren was gebeurd, zou het te laat zijn geweest!”

Dit is wat ontwikkelaars meestal doen. Ze luisteren naar hun klanten over de functies die ze nodig hebben en ontwikkelen vervolgens de code die dat doet, meestal onder enige tijdsdruk. Ze leveren de code af en gaan door naar de volgende taak. Maar ergens daar tussenin moeten ze samenwerken met andere ontwikkelaars. Er zijn interne coderingsstandaarden die moeten worden gevolgd, ze moeten de code schoon houden en ze moeten ervoor zorgen dat ze de code beter achterlaten dan deze was voordat ze eraan begonnen te werken. Om dit te bereiken moeten ze collega-ontwikkelaars vragen hun code te beoordelen, wat kan worden bereikt door een pull-verzoek te openen.

Laten we eerst eens kijken naar enkele tips en de belangrijke vragenmakers van pull-aanvragen moeten zichzelf afvragen:

1. Zou u het pull-verzoek goedkeuren als iemand anders het zou openen?
2. Hebt u elke regel code in de pull-request gecontroleerd? Is verandering x echt nodig?
3. Heeft u ervoor gezorgd dat de code voldoet aan de interne standaarden (opmaak, enz.)?
4. Heb je de code gecontroleerd op typefouten?
5. Gaat de volledige build door?
6. Is er voldaan aan de testnormen (unit-, API-tests)?
7. Zijn mijn commits gekoppeld aan het verhaal, zodat reviewers de business case kunnen begrijpen?

De andere betrokken partij is debeoordelaar van pull-aanvragen. Reviewers moeten het pull-verzoek goedkeuren en met hun goedkeuring garanderen dat de code voldoet aan het projectbeleid en doet wat hij moet doen.Meestal moet de recensent de volgende vragen beantwoorden:

1. Is de code schoon en zonder onnodige wijzigingen (lege regels, ongebruikte import)?
2. Doet de code wat het verhaal zegt dat het moet doen?
3. Is de code goed getest?
4. Voldoet de code aan de coderingsnormen?
5. Vergen veranderingen extra discussie, bijvoorbeeld input van een architect?
6. Kan ik enkele objectieve verbeteringen voorstellen of subjectieve observaties geven om de codeervaardigheden van iedereen die betrokken is bij de beoordeling van pull-aanvragen te verbeteren en de code te verbeteren?
7. Heb ik tot nu toe voldoende vertrouwen in de maker van de pull-aanvraag of moet ik de code uitchecken en volledig testen om de pull-aanvraag goed te keuren?

Voordat we al deze punten analyseren, moeten we begrijpen dat aPull Request Review is een proces waarin verschillende partijen tot een basisovereenkomst moeten komen. Dat betekent dat we allemaal moeten samenwerken en een aanvaardbare oplossing voor het probleem moeten vinden, zelfs als we het er niet helemaal mee eens zijn. Om dat te bereiken moeten we ons concentreren op een constructieve dialoog en begrijpen dat alle betrokkenen de beste bedoelingen hebben. Te vaak wordt dit idee vergeten, verworpen of opzettelijk genegeerd.

Richtlijnen voor makers

Het moeilijkste wat je kunt doen bij het schrijven van code, is proberen de code die je schrijft objectief te evalueren. Het is gemakkelijk om fouten in het werk van anderen te zien, maar moeilijk om onze eigen slechte praktijken te zien. Dus hoe bereik je een bepaald niveau van codekwaliteit als het lijkt alsof alleen al de definitie van codekwaliteit subjectief is, tenminste aan het begin van onze ontwikkelaarscarrière?

Er is maar één antwoord op deze vraag en het is vrij eenvoudig:ervaring. Als je een junior ontwikkelaar bent, overleg dan met je mentor en vraag om een ​​interne codebeoordeling, zelfs voordat je een pull-verzoek opent. Voor ervaren ontwikkelaars heb ik maar één tip: blader door je code en onthoud eerdere pull-verzoeken.Pas de aanbevelingen toe die u het vaakst ontvangt of geeften dat proces zou goede code moeten opleveren. Als u op deze manier over codewijzigingen nadenkt, gaat u onvermijdelijk over de wijzigingen nadenken alsof iemand anders ze heeft aangebracht.

Wanneer u anderen vraagt ​​uw code te beoordelen, moet u één ding begrijpen:reviewers zijn bereid een deel van hun tijd te besteden aan het beoordelen van uw wijzigingen. In ruil daarvoor moet je die veranderingen daadwerkelijk doorvoeren. Niet meer en niet minder. Je zou je moeten laten zienrespect voor hun tijd door te controlerende codetwee of zelfs drie keer.

Wanneer u code ontwikkelt, wijzigt u mogelijk enkele regels code of zelfs enkele bestanden, maar dan verandert u van gedachten en draait u die wijzigingen terug. Het komt vaak voor dat ontwikkelaars een aantal nieuw toegevoegde lege regels, witruimte, etc. “vergeten”. Voer deze wijzigingen alstublieft niet door. Verspil alstublieft de tijd van de recensent niet, en maak alstublieft geen rommel met geschiedenis. Het is begrijpelijk dat er tijdsdruk is, maar het nemen van deze sluiproutes betekent dat beoordelingen een omweg nemen en dat toekomstige beheerders van de code ook een omweg nemen.

De volgende tip gaat over het correct instellen van uw IDE. Stel uw formatter in, stel uw importvolgorde in, stel uw opslagacties in, stel uw algemene projectbeleid in. Update ze wanneer ze veranderen. Ontelbare keren wisselen ontwikkelaars tabbladen om voor witruimtes en vice versa, herschikken ze importorders en herformatteren ze code in het algemeen zonder enige toegevoegde waarde.Het herstellen van uw IDE-instellingen is een eenmalige klus en er mag geen excuus zijn om deze niet correct in te stellenom te voldoen aan de interne coderingsnormen.

Voordat u een pull-verzoek opent, moet u voorzichtig zijnlees uw codewijzigingen regel voor regel, letter voor letter. Controleer op typefouten en correct hoofdlettergebruik. Je corrigeert typefouten en, belangrijker nog, je leest de code goed, op dezelfde manier als reviewers dat doen, zodat je de kans hebt om je naamgeving, volgorde, etc. te heroverwegen. Heb je de beste methode en variabelenamen geselecteerd of zou er iets aan de hand zijn? van een betere naam? Zijn privémethoden waar ze horen te zijn?

Je bent geen brandweerman. Ik heb vaak een pull-request geopend waarbij de build niet slaagde. Soms voer je niet alle tests uit. Misschien schakelt u tijdelijk enkele kwaliteitscontroles uit. Het gebeurt en je leert. Zorg ervoor dat voordat u op die knop drukt, u "duwt".volledige bouwpassen. Een mislukte build treedt meestal op wanneer u opmerkingen over pull-aanvragen adresseert en enkele kleine wijzigingen aanbrengt, zodat u geen mislukte test verwacht. Wees niet verrast. Het overkomt iedereen. Ja, een productie-hotfix is ​​urgent, maar niet zo urgent dat u de volledige lokale build moet overslaan of slechte codewijzigingen moet aanbrengen.

Wat is het percentage code in uw huidige project dat moet worden afgedekt met unit-tests? Ken jij? Als het nummer niet is gedefinieerd, kunt u het beste contact opnemen met uw ontwikkelaarsmanager.

Wat belangrijk is om te begrijpen over het schrijven van tests is dat ze niet voor jou bedoeld zijn. Je hebt je functie gecodeerd en je weet hoe het werkt. Je hebt het getest en je tests zullen niet falen. Er zijn tests beschikbaar voor toekomstig werk aan uw code. Er komt een nieuwe ontwikkelaar, die wat code verandert, en tests zullen mislukken. Als je je werk goed hebt gedaan, kan hij snel vaststellen of de verandering de test heeft doorbroken of een nieuwe en onverwachte bug heeft geïntroduceerd.Zorg ervoor dat uw tests duidelijke, gemakkelijk leesbare, zelfstandige eenheden zijn die precies testen wat u bedoelde. Maak geen aannames over de code in uw tests, houd de tests klein en test één ding per test. Niemand wil een groot aantal afhankelijkheden doornemen om erachter te komen wat een test test of waarom deze faalt.

Zorg ervoor dat zodra een reviewer naar uw code kijkt, hij precies weet welk probleem u probeert op te lossen.Het commit-bericht moet verwijzen naar het gerelateerde verhaal en de verhaalbeschrijving moet accuraat zijn. Ik kan het aantal keren dat ik een verhaal heb geopend met de tekst "fix bug x" niet tellen. Je zou misschien willen zeggen dat iemand anders die titel heeft gemaakt, maar je moet begrijpen dat jij uiteindelijk degene bent die code schrijft. Jij bezit het. Als u alleen weet wat er moet gebeuren, kan niemand verifiëren dat uw code werkt zoals bedoeld. En als niemand je werk kan verifiëren, weet niemand of je een goede ontwikkelaar bent, zelfs je leidinggevende niet.

Richtlijnen van de recensent

Door de wijzigingen snel te controleren, krijgt u een algemene indruk van de pull-aanvraag die u gaat beoordelen. Je kunt zien dat de auteur haast had als de code vol staat met onnodige wijzigingen, enz. Dit helpt je te bepalen naar wat voor soort code je kijkt. Is het iets dat u snel kunt beoordelen en goedkeuren of vereisen de codewijzigingen een diepgaander gesprek met de auteur en dus een gepland tijdslot voor een beoordeling?

In mijn ervaringen,makers van pull-aanvragen die anderen respecteren, volgen een gemeenschappelijke reeks richtlijnen en zetten zich in voor het leveren van schonere en betere code, die sneller wordt beoordeeld. Natuurlijk voldoen sommige auteurs niet aan de beschrijving, maar we werken met meerdere auteurs, dus een individu moet zich aan de gemeenschappelijke regels houden, en niet andersom.

Het zou het beste zijn als u bepaalde wat de code zou moeten doen. Persoonlijk vind ik het leuk om te beginnen met het lezen van het verhaal in het bijbehorende ticket en vervolgens naar de toegangspunten in het systeem te kijken om te zien hoe de verandering aan de buitenkant wordt weerspiegeld. Om dat te begrijpen heb je misschien enige algemene kennis nodig van de hele module waar de code deel van uitmaakt. Dit betekentu moet eerst begrijpen wat u beoordeeltvoordat u beslist of de code goed is of niet. Heeft iemand iets eenvoudigs veranderd in de 'if'-instructie? U moet nog steeds de hele module kennen, zelfs als de wijzigingen eenvoudig zijn. Die gewijzigde verklaring kan enorme gevolgen hebben en niet alle bugs kunnen worden opgelost met wijzigingen in een enkele regel.

Nadat u ervoor heeft gezorgd dat de codewijzigingsset netjes en schoon is en u begrijpt waar de codewijziging over moet gaan, kunt u beginnen met het graven in de code. U kunt de gewijzigde methoden en de bijbehorende unittests bekijken.Als de unit-tests op de juiste manier worden benoemd en geschreven, wordt dit u altijd duidelijk gemaaktwat de invoergegevens zijn enhoe een stukje code zich zou moeten gedragen. Nadat u dit allemaal heeft gecontroleerd, moet u een goed begrip hebben van de code en de bedoelingen van de auteur bij het schrijven ervan. Hierna zou je eindelijk klaar moeten zijn om de auteur wat feedback te geven.

Normaal gesproken is mijn eerste feedback het geven van commentaar op wat voor de hand ligt: ​​typfouten, onjuist benoemde methoden, mogelijke fouten veroorzaakt door randgevallen, enzovoort. Het idee hier is datde code voldoet aan de basisnormen, kunt u beginnen met het onderzoeken van kanttekeningen bij de implementatie. Zelfs als de code uiteindelijk totaal anders is, zal de oorspronkelijke auteur nog steeds de basisvaardigheden van ons vak leren en verbeteren. Bij ervaren auteurs zijn er op dit moment meestal minder van dergelijke problemen.

Nu kunt u beginnen met het bekijken van de code in het kader van de hele module. Is de code correct geplaatst? Zijn de juiste codeerpatronen gebruikt? Zijn edge cases goed doordacht en is code generiek genoeg of moet het minder generiek zijn? Heeft de code op onverwachte manieren invloed op de hele module? Kan dit een negatief effect hebben op andere aspecten van ons hele ecosysteem, bijvoorbeeld vanuit prestatieperspectief? Is er voldaan aan de veiligheidsnormen? Hier begint de discussie met de auteur.Deze functie beperkt altijd het zicht van de maker en het is jouw taak om dat vak een beetje groter te maken.

U moet speciale aandacht besteden aan de discussie die plaatsvindt tijdens het beoordelingsproces van pull-aanvragen. Je moet ervoor zorgen dat er geldige argumenten worden aangevoerd en indien nodig architecten om hun inbreng vragen. Als we allemaal een werkomgeving willen hebben die kwaliteitscode produceert, dan zijn dezeargumenten moeten duidelijk en begrepen zijnom dat doel te bereiken. Te vaak raken auteurs en recensenten verwikkeld in zinloze discussies en beginnen alle betrokken partijen opmerkingen op te vatten als persoonlijke aanvallen, en niet als objectieve feedback over de voorgestelde codewijziging.

Ik was ooit betrokken bij een project waarbij reviewers met elkaar concurreerden wie als eerste een pull-verzoek zou goedkeuren. Hoewel ik erken dat er triviale codewijzigingen zijn die geen speciale aandacht behoeven, moeten we elke afzonderlijke codewijziging in detail bekijken. We moeten anderen laten zien dat we de code in detail beoordelen en dat we te vertrouwen zijn. Een keer andersontwikkelaarsbegrijp dat, zijzal automatisch betere code produceren om ervoor te zorgen dat verzoeken tijdig worden beoordeeld. Als er deadlines zijn voor het leveren van code, moeten we de communicatie en misschien het gebruik ervan verbeterenPaar programmerenom eventuele codeproblemen op te lossen. Wat u niet moet doen, is bezwijken onder de tijdsdruk en daardoor de kwaliteitsnormen voor code verlagen. Je hoeft maar één keer een uitzondering te maken en het kan al snel de standaard worden..

Het gaat niet om jou. Het gaat om het proces!

Het beoordelen van pull-aanvragen is een proces dat speciale aandacht verdient, omdat het de belangrijkste plaats van interactie rondom ons werk is. Iedereen was ooit een junior ontwikkelaar, en zelfs met Stack Overflow en Google hebben we feedback van collega-senior ontwikkelaars nodig om onze codeervaardigheden te verbeteren.Als het beoordelingsproces gebaseerd is op argumenten en verdiensten, zal iedereen er profijt van hebben. Onze klanten krijgen een kwaliteitsproduct, wij als ontwikkelaars leren van elkaar en toekomstige codebeheerders zullen hun werk sneller en beter doen. Wij, de ontwikkelaars, moeten eerst begrijpen dat we geen brandweerlieden zijn. Als we code afleveren die bezuinigend is, zal deze ons blijven achtervolgen. En als we code van iemand anders erven, zullen we dankbaar zijn als de codekwaliteit op een acceptabel niveau is. We zullen nog steeds elke keer de code van andere ontwikkelaars willen herschrijven; de code heeft geen hotfix of hotfix of bugfix nodig.

U kunt naar uw project en het vastgestelde codebeoordelingsproces kijken en proberen dit te verbeteren. Kijk naar de toon van het gesprek in de reacties. Zorg ervoor dat u argumenten en ideeën uitwisselt, zodat optimale code kan worden geleverd. U hoeft geen slechte code te tolereren, maar communiceer gewoon duidelijk waarom u denkt dat er iets mis is. Te vaak beschouwen mensen opmerkingen als persoonlijke aanvallen en als je een cultuur kunt creëren waarin de focus ligt op de code en niet op de mensen achter de code, ga je de goede kant op.Maar zoals bij de meeste dingen is de echte uitdaging het behoud ervan. Veel succes op dat pad!