La revue de code, c'est important !

a marqué ce sujet comme résolu.

Salut tout le monde, J’ai écrit un petit article sur la revue de code. Si vous avez des remarques, je suis preneur ! Merci !

Tout le monde se secoue ! :D

J’ai commencé (vendredi 02 novembre 2018 à 13h19) la rédaction d’un article au doux nom de « La revue de code, c’est important ! » et j’ai pour objectif de proposer en validation un texte aux petits oignons. Je fais donc appel à votre bonté sans limites pour dénicher le moindre pépin, que ce soit à propos du fond ou de la forme. Vous pourrez consulter la bêta à votre guise à l’adresse suivante :

À présent, c’est à vous !

Merci !

+6 -0

C’est tout gentil comme article. Pas grand chose à dire, à part que ça nécessite une relecture :D pour des phrases maladroites. J’aurais également rajouté que ça permet d’augmenter la cohérence dans le projet, les conventions de code. Le respect de standard/iso certification ou d’audits. Et peut-être de mieux fixer les attentes pour les jeunes. Courage !

+2 -0

C’est un article intéressant sur un bon sujet, merci !

Quelques remarques.

  • Globalement l’article ne présuppose pas que l’audience travaille dans le cas d’un projet libre principalement bénévole, ou dans le cas d’un projet d’entreprise principalement managé de façon hiérarchique. C’est bien, parce que ça permet aux oersonnes des deux catégories de s’identifier, de se sentir concerné-e. Le seul moment qui m’a fait tiquer est le partie sur "comprendre le besoin du client", qui est exprimé dans un vocabulaire de société de service. Ce serait bien de récrire cette phrase pour la rendre plus neutre sur le cadre (comprendre/analyser le besoin à la source du changement, et comparer avec la solution proposées; ça fait aussi sens dans un projet libre collaboratif, même quand le besoin vient de l’auteur du changement proposé). Globalement faire attention à l’emploi du mot "client".

  • L’article se disperse un peu sur des sujets qui ne sont pas la revue de code. La revue de code, c’est la relecture par un humain, et après que le patch/changement est considéré par son auteur comme prêt pour une relecture. L’intégration continue et la programmation en binôme ne sont pas des revues de code, et c’est une légère faute de ton texte d’en parler comme si ça en faisait partie. Par contre, la "relecture par dessus l’épaule" est une forme de revue de code si c’est fait après-coup, plutôt que pendant l’écriture du patch.

  • Mon expérience pratique de la revue de code est qu’en terme de "défauts" évités, elle est surtout pertinente pour garantir les qualités du code qui concernent les humains: propreté, maintenabilité, cohérence avec le reste du programme, etc. Elle peut attraper des problèmes subtils auxquels les humains experts vont penser, trouver d’autres bugs (est-ce que ton problème se pose aussi dans les cas X et Y ?), ou suggérer d’autres changements au programme (au passage il serait bien de …). Par contre elle est très peu efficace pour attraper certains bugs "bêtes" comme la cassure de compatibilité sur tel ou tel système, le non-respect de conventions de style, ou tout simplement une erreur de compilation quelque part. (À moins que le projet ait une structure très stricte avec des checklists sur ces points précis, et encore.) Il y a des trucs que les humains ne sont pas bons pour vérifier, et c’est là que la testsuite et l’intégration continuent (CI) prennent tout leur intérêt. Plutôt que de présenter le CI comme une sous-composante de la revue de code, je préfère dire qu’elles sont complémentaires (relectures par un humain, vérifications automatiques par des machines). Les outils automatisés peuvent aussi fournir des aides à la revue et discussion d’un patch (alarmes automatiques sur certaints points, désignation des reviewers, lancements de tests automatisés spécialisés par un expert humain, etc.).

  • En général, dans mon expérience, la revue de code se fait seulement à partir du diff, sans builder/tester le projet à la main. Quand on demande aux gens de builder/tester, on parle plutôt de "Quality AnalysisAssurance" (QA) que de revue de code. Le test réel du code par un reviewer ne se fera que dans des cas spécifiques, pour des changements importants ou subtils. Le reste du temps c’est plutôt "je fais confiance à la CI" ou bien "je me pose la question X mais j’ai la flemme de tester, rajoute donc un test unitaire pour cela dans la PR". Je trouve que c’est un peu abusif de dire, comme tu l’as fait, "[le relecteur] va également tester le code et donc : […]". Autant être réaliste.

  • Le mot "question" n’apparaît pas assez dans ton article. Le fait de poser des questions est un des rôles les plus importants des relecteurs. Un non-expert peut poser une question sur un aspect du changement qu’il ne comprend pas, ça ne veut pas forcément dire qu’il y a un problème / une erreur à cet endroit, mais cela donne en général lieu à du partage de connaissances et de la production de documentation (dans la discussion sur le changement, mais parfois aussi comme un nouveau commentaire dans le code à cet endroit, dans le manuel, etc.). Quand je décris aux gens ce qu’ils peuvent faire pour aider un projet, je parle de la revue de code en disant : choisit une PR qui t’intéresse, lit le patch et, dès que tu te poses une question à laquelle tu ne peux pas répondre toi-même, pose-la.

J’ai aussi trouvé une typo:

Si une deadline est proche, la revue de code va être précipité.

précipitée.

+3 -0

Merci pour tes remarques @gasche, je vais essayer de prendre tout ça en compte.

Pour ce qui est de la code review vs QA, en effet, c’était un peu abusif de dire que le relecteur va tester. C’est plutôt basé sur mon expérience personnelle. Je m’occupe notamment des relectures et de tout ce qui est tests avant intégration. Mais globalement, je fais de la QA en effet. ;)

+0 -0
Ce sujet est verrouillé.