Revues de code


#1

Avec Manu, Jonathan et (vers la fin) Etienne on s’est demandé “Comment tu fais des revues de code sur Github ?”

Manu souligne les questions suivantes qu’a soulevé la session:

  • l’importance du contexte, c’est à dire:
    • la prise de conscience de mes attentes en tant que relecteur: qu’est-ce que je cherche à faire ?
    • ma perception de ce que l’auteur·trice de la PR attend de moi
    • un ensemble de codes sociaux: qui relit qui ? quelle est ma relation avec l’implémenteur·trice ?
    • mon appréciation des compétences de la personne qui a poussé la PR (relativement au projet, ou relativement au développement en général)

Mes propres réactions sont celles d’une personne qui découvre le code pour la première fois. Je prends conscience d’un processus en plusieurs temps. D’abord il y a des perceptions immédiates, notamment

  • quel est le langage de programmation ?
  • combien de fichiers modifiés, combien de commits ?
  • combien de lignes ajoutées/supprimées ?

Manu parle de calibration: certains éléments qui sont intrinsèques à la PR et qui vont moduler la façon de l’aborder, et d’autres qui sont relatifs au contexte plus large. Est-ce que cette PR fait une seule chose (ajouter une fonctionnalité) ou plusieurs (ajouter une fonctionnalité ET apporter un changement de conception ou d’architecture).

Ensuite je m’interroge sur mes intentions et je pars sur un comportement plus analytique.

On (pour certaines valeurs de “on”) a de plus en plus tendance à confondre “revue de code” et “relecture de PR en vue de les accepter”, et ça vaut peut-être le coup de faire la différence. D’ailleurs que veut dire accepter une PR? On part de l’hypothèse que l’intention de l’implémenteur est de voir son code accepté (mergé), et que c’est aussi la finalité pour le relecteur. It takes two to tango: on est dans une danse, consentie, entre deux personnes avec une même finalité. (En fait on peut aussi avoir connu des occasions où ce n’est pas le cas… Je me souviens d’un cas il y a quelques semaines ou j’ai entendu le compliment “elle est bien ta PR proof-of-concept”, avec le sous-entendu qu’elle pouvait ne pas être acceptée.)

On peut distinguer plusieurs intentions:

  • vouloir garantir la cohérence ou l’homogénéité du code dans un même dépôt
  • offrir un feedback “optionnel” : voilà ce que je pense de ce code, tu en fais ce que tu veux (de mon feedback)
  • aider quelqu’un à progresser, par exemple une personne junior qui fait ses premières PR (donc on part facilement du principe que ce ne sera pas parfait)
  • engager sa responsabilité : si j’accepte une PR je m’en porte garant comme si j’avais moi-même écrit ce code (Jonathan fait l’hypothèse que cette histoire de responsabilité est le symptôme d’une culture de blâme ; Manu que c’est aussi une question d’entraînement à prendre ses responsabilités)

J’ai essayé de matérialiser certains aspects de cet arbitrage entre les responsabilités de l’implémenteur·trice et de la personne qui relit par un beau diagramme 2x2 mais ça n’a pas été tout à fait concluant. Reviewer vs implémenteur croisé avec feedback sur la qualité vs garde-fou contre les bugs. Mais on peut aussi considérer forme vs fond, recette fonctionnelle vs qualité interne… Est-ce que le relecteur·trice d’une PR a une “obligation” de faire tourner le produit en local et vérifier que tout marche encore ? S’il ou elle ne le fait pas et qu’on constate un bug après la mise en prod, qui a-t-on “le droit” d’engueuler. Pour valider une PR il peut être attendu de faire plus que relire le code. L’outillage peut diminuer la friction à faire ça, par exemple les “review apps” proposés par Heroku, Netlify etc. permettent d’avoir une pré-prod automatique par PR, en complément de l’intégration continue.

Il y a aussi une incidence de la forme: sur Github on ne voit que les diffs (donc souvent une toute petite partie du code dans sa globalité), or “relire un delta” et “revue de code” ce n’est pas la même chose. En anglais j’appellerais ça le “keyhole problem”, une vision du code par le trou de la serrure. Par exemple pour Jonathan, c’est une préoccupation très importante de repérer dans une PR les éventuelles divergences par rapport à “ce qui se fait ailleurs dans le code”, et aller vérifier les autres fichiers qui devraient ressembler à ce qu’on relit. On est sur une logique de jurisprudence : il n’y a pas forcément une bonne façon d’écrire le code, et je peux préférer une manière plutôt qu’une autre, mais c’est plus important de respecter une cohérence globale que de voir ses chouchous adoptés, d’imposer son style. (On pense au jugement de Salomon…)

Les décalages d’intentions que toute cette palette peut provoquer sont sources de tensions. Parmi ces tensions, on trouve aussi la tension entre un regard critique et une intention bienveillante. Contrairement à ce que j’ai appris par exemple via le bouquin de Gabriel “Writer’s Workshop” où on offre du feedback comme un cadeau, on est ici en co-responsabilité : ce n’est pas un seul auteur qui publie et qui est souverain dans la décision de prendre ou ne pas prendre en compte un feedback, on est une équipe et ça change tout.

On voit que c’est déjà assez compliqué alors qu’on n’a traité que l’intention. Quand on passe à la pratique, là encore le contexte change pas mal de choses. Par exemple, mon ordre de lecture n’est pas le même que celui de Manu qui connait déjà le code. J’ai tendance à lire de haut en bas dans l’ordre (alphabétique) des fichiers, en absorbant sans jugement ce que je vois jusqu’à ce que j’arrive à me faire une idée du sens de la PR; Manu sait comment la pièce est rangée et ouvre les tiroirs dans un ordre qui a du sens pour lui.

D’ailleurs Manu a déjà fourni quelques commentaires sur cette PR, et comme c’est sa deuxième lecture une de ses intentions est de voir s’ils ont été pris en compte.

D’ailleurs à quel point le commentaire est-il la “bonne” façon de réagir à une PR ? L’outil Github impose le commentaire comme unité d’observation, et invite à se focaliser là-dessus, ce qui peut être au détriment de remarques globales, ou bien induire des attentes de comportement (qui doit marquer un commentaire comme “résolu”, est-ce que c’est acceptable de ne pas le faire, quel bénéfice à le faire, etc.), mais aussi masquer le fait qu’on peut intervenir autrement sur une PR: ajouter des commits (ou, nouvelle feature de Github, des suggestions). Ou pourquoi pas aller parler avec l’auteur du code !

Jonathan rappelle que l’outil induit aussi une vision “à distance” de la relecture du code, on suppose que l’auteur·trice n’est pas présent. On peut faire les choses autrement, par exemple binômer (ou mobber, coucou Christophe) pendant l’écriture du code. Comme d’habitude, il n’y a pas de “problème” technique qui ne soit d’abord un effet de la relation entre les personnes et la qualité de ces relations.

Parmi quelques questions annexes: combien de passes ou d’itération prévoit-on de faire sur une PR ? On se comportera différemment selon qu’on pense “une seule” ou “autant qu’il faut pour que ce soit parfait”. A combien de personnes propose-t-on de relire une même PR ? Chez PIX où travaille Jonathan, on s’attend à ce que ça soit au moins 2 (avec un flou sur le fait qu’avoir binômé compte pour une première relecture).

Dernières notes en vrac;

  • signaux faibles / hints checklist
  • extra mile
  • intégrité

On a globalement beaucoup apprécié et appris de cette session, en partageant nos pratiques dans une situation concrète et quotidienne.