Souvent, lorsque l'on mets en place un système de Pull Request, la question arrive vite : "Pourquoi on perd un temps fou avec ça ?". Je suis pour ma part un fan inconditionnel du système de PullRequest. Dans cet article pas trop technique, je ferais un rappel de ce que c'est et je vous donnerai le fond de ma pensée sur le sujet.

Qu'est-ce que c'est, une PullRequest ?

Une PullRequest - PR (aussi appelée "requête de tirage" si vous aimez les termes techniques mal traduits) est tout simplement une demande de relecture de code par un pair développeur avant le merge d'une branche sur une autre.

Ce système est particulièrement utile si vous utilisez une gestion des branches à la GitFlow : 

• Vous avez une branche principale (appelons la develop) qui sert de référence : ce qui est dedans est dans un état assez stable et contient les fonctionnalités des prochaines livraisons.
• Vous faites une branche par fonctionnalité qui part de develop. 

Une PullRequest va consister à déclarer : j'aimerais bien pousser sur la branche develop cette fonctionnalité "A" que j'ai développé dans ma branche, vous pouvez regarder ? Cette fonctionnalité ne sera alors mergée sur develop qu'une fois la PR complétée. Cette complétion n'étant possible que si certaines conditions sont réunies. 

Une PR n'est pas figée : il est possible de créer de nouveaux commits et de les pousser sur la branche source sans avoir à recréer de nouvelle PR. Il s'agit bien d'un processus qui tend vers un meilleur code à pousser sur la branche principale.

Comment j'en fais une dans Azure Dev Ops ?

La notion de PullRequest est tellement intégrée à Azure Dev Ops qu'elle possède un onglet dédié dans la section Azure Repos. Sur cette page, vous pouvez voir les différentes PR en attente de validation, leur statut et le nombre de commentaires déjà apposés.

Pour créer une nouvelle PullRequest, il faudra passer par le bouton "New pull request" en haut à droite, et il vous sera alors demandé de choisir la branche source (celle où vous avez fait la nouvelle fonctionnalité) et la branche cible (là où vous voulez que cela soit mergé). Une fois ces informations saisies, il faudra indiquer un titre, une description et vous pourrez choisir des reviewers (des personnes qu'il faut notifier de la demande de relecture) et associer des work items pour indiquer ce que vous traitez dans cette PR. Il est également possible de créer une PR en mode "brouillon" pour avoir la visibilité des modifications sans demander aux autres de relire (et cela ne déclenche pas non plus les builds).

VSTS fait alors sous le capot un merge avec la branche cible et propose en dessous la liste des fichiers impactés par l'intégration de cette branche source dans la branche cible.

Relecture d'une PR

La relecture d'une PR dans Azure DevOps (attention c'est différent dans d'autres gestionnaires de code source comme GitHub) est assez bien faite :

• Un onglet "Overview" qui présente les commentaires et modifications apportées sur la branche source de la PR.
• Un onglet "Files" qui présente la liste des fichiers impactés. Les modifications faites sur chacun sont mises en exergue via un code couleur simple (rouge = supprimé, vert = ajouté / modifié).
• Un onglet "Updates" qui présente les différents pushs (des groupes de commits donc) faits sur la branche source.
• Un onglet "Commits" qui présente les différents commits poussés sur la branche source.

Relire une PR consiste à parcourir les fichiers modifiés et s'assurer de plusieurs choses :

1. Que le code écrit n'introduit pas de bugs.
2. Que le code écrit respecte les conventions (nommage, etc.) établies sur le projet.
3. Que le code écrit est complet : y a t'il bien les tests unitaires ou d'intégrations nécessaires ?
4. Qu'il n'y a pas de mise en commun de code dupliqué à faire ou autres améliorations potentielles.
5. Que le code écrit correspond à ce qui est décrit dans l'US associée à la tâche traitée par cette PR.
6. S'assurer que l'on comprend bien ce qui est fait. Si ce n'est pas le cas, c'est l'occasion de demander des commentaires dans le code!
7. Faire des remarques de manière générale.

Les différentes remarques sont faites via des commentaires directement sur le code. Chaque commentaire est un vrai fil de discussion et porte un statut indiquant s'il est résolu ou non. Il est très important d'être bienveillant dans nos relectures de code !

Finalement il faut donner son avis global sur la PR :

• Approved : je n'ai rien à dire c'est parfait.
• Approved With Suggestions : Cela me va mais quelques petits points non bloquants sont détectés.
• Wait for author : j'ai fait des commentaires qui me semble nécessaire de prendre en compte avant que cela ne passe sur la branche cible.
• Reject : Non mais allo quoi ? Qu'est ce que tu as fait ??

Je pense qu'il est très important de relire aussi soi-même ses propres PR ! La présentation sous forme de fichiers modifiés permet de se focaliser sur les changements faits alors que c'est plus difficile au sein de l'IDE. La bonne pratique que je recommande et de créer sa PR en brouillon (Draft), de la relire soi même et de ne faire la publication (mise à disposition aux autres développeurs) qu'après sa propre passe de relecture. Cela fait gagner du temps à tout le monde.

Les Conditions de complétion

Un autre aspect intéressant est la possibilité de mettre des conditions sur la complétion d'une PR. Pour cela on va sur la liste des branches du repository et on sélectionne "Policies" dans le menu adéquat :

Sur l'écran de configuration suivant on peut choisir plusieurs conditions :

• Demander un nombre minimum de relecteurs en spécifiant si on peut soit même approuver sa PR. C'est une bonne pratique et il faut configurer le nombre minimum de relecteurs en fonction de la taille de vos équipes.
• Demander la présence d'un WorkItem lié : cela permet de spécifier au mieux à quoi correspond votre PR et le workitem aura lui aussi un lien vers la PR qui correspond à son implémentation.
• Demander que chaque commentaire fait ne soit plus au statut Actif. Personnellement je l'active à chaque fois, cela permet de ne jamais en oublier un par mégarde.
• Demander qu'au moins un relecteur d'un groupe de personnes désigné approuve la PR.

Aussi, ma condition préférée : demander qu'une build passe en succès pour qu'une PR puisse être complétée. J'en ai d'ailleurs parlé dans un précédent article.

Il est possible de spécifier qu'une build ne doit être obligatoire que si certains chemins du repository sont modifiés. Chaque modification faite sur la branche cible impacte potentiellement le code que vous allez pousser car un merge sera fait (en automatique si possible, avec des effets quelques fois désastreux). Aussi, il est nécessaire de spécifier une date d'expiration de chaque build. J'aime demander une expiration à chaque modification de la branche cible pour réduire les risques, mais mes collègues n'aiment pas trop voir leurs builds expirer si souvent.

Petite ou grosse PR ?

La taille de votre PR va être directement dépendante de la fonctionnalité que vous êtes en train de développer. Personnellement, je pense qu'il est plus intéressant de faire des petites PRs que des grosses PRs avec plein de fonctionnalités : il y a moins de code impacté et le travail des relecteurs en sera facilité et plus précis. Pour réaliser cela, il est important de pouvoir découper le besoin fonctionnel en plusieurs tâches techniques indépendantes.

Si le travail d'une tâche nécessite le code d'une branche en cours de relecture, cela n'est pas un problème : faites partir votre nouvelle branche de la branche en cours de relecture puis une fois qu'elle sera validée et présente sur la branche cible, il ne vous restera plus qu'à faire un rebase depuis celle-ci sur votre branche de développement de la fonctionnalité suivante.

Mais pourquoi on en fait ?

C'est vrai qu'il y a quand même des frictions ajoutées :

• Il faut que quelqu'un trouve le temps de relire ton code.
• Il faut que tu trouves le temps de prendre en compte les commentaires.
• Il faut attendre que les builds obligatoires passent. 
• Il faut ensuite que ça soit poussé sur la branche cible, que la build de CI sur cette branche passe pour qu'enfin cela soit déployé sur un environnement de test.

Mais au final on apporte plein d'avantages que je trouve miiiille fois plus intéressants  !

La branche cible est toujours propre - les problèmes de build sont détectés en amont

Etant donné que pour que du code arrive sur la branche cible, il est nécessaire qu'il soit relu et qu'il compile, il y a beaucoup moins de risques qu'un problème soit introduit sur la branche cible (que l'on veut la plus sèche possible). Si un souci est introduit, on le saura directement dans la PR et surtout le périmètre des causes possibles en sera réduit à uniquement les modifications faites (encore un argument pour avoir des petites PRs). Bien sûr, il est toujours possible que cela arrive mais le risque est bien moins grand.

Quelqu'un d'autre interprète la demande fonctionnelle

Une US ou une tâche étant écrite par un humain et nous, pauvres codeurs, étant aussi presque humains, tout est sujet à interprétation. Il n'est pas rare que le besoin exprimé d'une certaine manière ne soit pas forcément interprété de la même façon par le développeur qui la réalise. C'est même plutôt souvent le cas... En demandant à d'autres personnes de comprendre l'US ou la tâche initiale pour vérifier que vous avez fait le bon travail permet d'avoir un second avis sur votre compréhension de l'US et de limiter les risques d'interprétation divergente d'une même demande.

Par exemple, vous voyez bien tous comme moi une place de parking pour des personnes aimant les barbecues ? : 

Vous n'êtes plus le seul spécialiste d'une partie du code

Il arrive, à mon grand regret, que le temps passant, certaines personnes deviennent spécialistes d'une certaine partie du code de votre solution technique (par exemple, Benoît F. est le spécialiste de l'upload de fichiers :D). C'est bien dommage : il n'y a que lui qui peut intervenir sur cette partie et s'il n'est pas là, patatras, il faut attendre son retour. 

Avoir des PR limite ce phénomène : comme le code est relu par tout le monde, chaque partie est connue de tous, même de manière superficielle. La connaissance est ventilée sur toute l'équipe.

Vous apprenez des nouvelles choses 

Il n'est pas rare qu'un développeur venant d'arriver sur le projet ne soit qu'optionnel et pas obligatoire sur la validation d'une PR. Bien sûr dans la foulée, avec des yeux tristes, au sujet des relectures de PR :

Ce même développeur en profitera pourtant pour découvrir de manière empirique les différentes bonnes pratiques et conventions appliquées sur le projet. Elles sont souvent documentées, mais nous n'avons pas toujours le temps de tout tracer de manière précise et un cas pratique est toujours plus simple à comprendre. 

De manière générale, tout le monde ne travaillant pas de la même façon, on apprend beaucoup des relectures de codes d'autres personnes. J'ai par exemple récemment découvert qu'on pouvait écrire les constantes numériques de manière beaucoup plus lisible en relisant une PR d'Alexandre. Mais c'est aussi plein de petits détails, de façons de faire, qui sont apprises au cours des relectures.

Beaucoup de temps est gagné

Il est toujours plus simple de corriger un problème avant qu'il n'apparaisse qu'après son introduction ("mieux vaux prévenir que guérir, tout ça, tout ça"). Une PR est justement le moyen de détecter qu'un bug est introduit et de faire gagner du temps que l'on aurait passé à investiguer un problème et à le corriger. Un institut focalisé sur la qualité de code estime que la revue de code, et donc les PR, font gagner 33 heures par heure de relecture ! Je vais le mettre en gros juste en dessous, ça fait toujours bien dans un article.

Je ne sais pas trop comment on faisait avant !

Pour vous détendre un peu après cette lecture, cliquez ici..