10 étapes universelles pour la révision du code open source

Vous êtes-vous déjà retrouvé dans une situation où vous deviez faire une revue de code mais ne compreniez pas entièrement le projet ? Peut-être que vous ne l’avez pas examiné pour éviter d’avoir l’air de ne pas savoir ce que vous faisiez.

Cet article vous assure qu’il existe un meilleur moyen. Vous n’avez pas besoin de tout savoir pour fournir une révision du code. En fait, d’après mon expérience, c’est assez courant.

Je me souviens quand j’ai rejoint Red Hat en tant que stagiaire et qu’on m’a demandé d’aider à la révision du code. Nous avons utilisé un système de votes +1 ou -1, et j’ai d’abord été très hésitant à intervenir. Je me suis demandé si, lorsque je donnais un +1 sur un changement, mais que quelqu’un d’autre votait -1, aurais-je l’air stupide ?

Que se passe-t-il si quelqu’un vote -1 sur un changement auquel vous votez +1 ? La réponse est rien ! Vous avez peut-être manqué un détail que l’autre personne a remarqué. Ce n’est pas la fin du monde. C’est pourquoi nous avons ce système de vote. Comme le reste de l’open source, la fusion de code est un effort collaboratif.

Dernièrement, j’ai été tellement inondé de revues de code que j’ai du mal à les suivre. J’ai également remarqué que le nombre de contributeurs faisant ces critiques diminuait régulièrement.

Pour cette raison, j’écris sur mon point de vue sur l’écriture d’une revue de code. Dans cet article, je vais partager quelques trucs et astuces utiles. Je vais vous montrer quelques questions que vous devriez vous poser et quelques idées sur ce qu’il faut rechercher lors d’une revue de code.

A quoi sert une revue de code ?

Avez-vous déjà écrit un patch vraiment simple ? Quelque chose que vous pensez est si trivial qu’il ne nécessite pas d’examen ? Peut-être que vous l’avez fusionné tout de suite. Plus tard, il s’avère qu’il y a eu une erreur, quelque chose d’évident ou de stupide, comme une mauvaise indentation ou quelques lignes de code dupliquées au lieu d’un appel de fonction (oui, je parle d’expérience !).

Une révision du code par quelqu’un d’autre aurait détecté ces choses.

Le but d’une révision de code est d’apporter une nouvelle paire d’yeux avec une nouvelle perspective sur le problème que vous essayez de résoudre. Ce nouveau contexte est exactement la raison pour laquelle une révision du code est cruciale.

Vous pensez peut-être que vous devez être un expert du langage pour réviser le code de quelqu’un d’autre, le projet ou les deux. Voici un secret que tous les réviseurs de code veulent que vous sachiez : c’est faux ! Vous n’avez pas besoin de bien comprendre le projet ou le langage pour apporter une nouvelle perspective sur un changement. Il existe un processus universel de révision du code.

Le processus universel d’une revue de code

Voici mon processus de révision du code, regroupé en quelques points. Le processus fournit des questions que je me pose pour m’aider à me concentrer sur un changement de code et ses conséquences. Vous n’avez pas besoin d’aller dans cet ordre spécifique. S’il y a une étape, vous ne pouvez pas l’exécuter pour une raison quelconque, passez simplement à une autre étape.

1. Comprendre le changement, ce qu’il essaie de résoudre et pourquoi

L’explication de la raison pour laquelle le changement est nécessaire et tout contexte pertinent doivent figurer dans le message de validation. Si ce n’est pas le cas, demandez-le et n’hésitez pas à -1 jusqu’à ce qu’il soit fourni.

Est-ce quelque chose qui doit être résolu ? Est-ce quelque chose sur lequel le projet devrait se concentrer, ou est-ce complètement hors de portée ?

2. Comment implémenteriez-vous la solution ? Serait-ce différent ?

À ce stade, vous savez en quoi consiste le changement de code. Comment auriez-vous fait ? Pensez-y avant d’examiner le changement en détail. Si la solution que vous avez en tête est différente de celle que vous examinez et que vous pensez qu’elle est meilleure, mentionnez-la dans l’examen. Vous n’avez pas besoin de -1 ; il suffit de demander pourquoi l’auteur n’est pas allé dans cette direction et de voir comment la discussion évolue.

3. Exécutez le code avec et sans le changement

Je mets généralement quelques points d’arrêt dans le code, je l’exécute et j’inspecte comment le nouveau code interagit avec le reste.

Si vous ne pouvez pas exécuter tout le code, essayez de copier la fonction contenant le nouveau code dans un nouveau fichier local, simulez les données d’entrée et exécutez-la. Ceci est utile lorsque vous ne savez pas comment exécuter l’ensemble du projet ou lorsqu’il nécessite un environnement spécifique auquel vous n’avez pas accès.

4. Le nouveau code peut-il casser quelque chose ?

Je veux dire, vraiment n’importe quoi. Pensez aux conséquences.

Dans le cas d’une nouvelle option de ligne de commande, sera-t-elle toujours acceptée par la cible ?

Une situation peut-elle se produire lorsque l’option ne serait pas acceptée ou qu’elle pourrait entrer en conflit avec quelque chose ?

C’est peut-être une nouvelle importation. La nouvelle bibliothèque, et éventuellement une nouvelle dépendance, est-elle disponible dans les anciennes versions ou systèmes pour lesquels vous expédiez le projet ?

Qu’en est-il de la sécurité ? La nouvelle dépendance est-elle sûre à utiliser ? Le moins que vous puissiez faire est de lancer une recherche rapide sur Internet pour le savoir. Recherchez également les avertissements dans le journal de la console. Parfois, il existe des méthodes plus sécurisées dans la même bibliothèque.

5. Le code est-il efficace ?

Vous avez déterminé que la solution proposée est probablement correcte. Il est maintenant temps de vérifier le code lui-même, son efficacité et sa nécessité.

Vérifiez le style du nouveau code. Est-ce que cela correspond au style du projet ? Tout projet open source a (ou devrait avoir) un document informant les (nouveaux) contributeurs des styles et bonnes pratiques suivis par le projet.

Par exemple, chaque projet de la communauté OpenStack a un fichier HACKING.rst. Il y a souvent aussi un guide pour les nouveaux contributeurs avec toutes les informations indispensables.

6. Vérifiez que toutes les nouvelles variables et importations sont utilisées

Souvent, il y a eu de nombreuses itérations du code que vous révisez, et parfois la version finale est très différente de quand elle a commencé. Il est facile d’oublier une importation ou une nouvelle variable qui était nécessaire dans une ancienne version du nouveau code. L’automatisation vérifie généralement ces éléments à l’aide d’outils de filtrage comme flake8 dans le cas du code Python.

Pouvez-vous réécrire le code sans déclarer de nouvelles variables ? Eh bien, généralement, oui, mais la question est de savoir si c’est mieux ainsi. Apporte-t-il un bénéfice ? Le but n’est pas de créer autant de one-liners que possible. Le but est d’écrire un code à la fois efficace et facile à lire.

7. Les nouvelles fonctions ou méthodes sont-elles nécessaires ?

Existe-t-il une fonction similaire pouvant être réutilisée quelque part dans le projet ? Cela vaut toujours la peine d’aider à éviter de réinventer la roue et de réimplémenter une logique déjà définie.

8. Existe-t-il des tests unitaires ?

Si le patch ajoute une nouvelle fonction ou une nouvelle logique dans une fonction, il doit également inclure de nouveaux tests unitaires pour cela. C’est toujours mieux lorsque l’auteur d’une nouvelle fonction écrit également des tests unitaires pour celle-ci.

9. Vérifier la refactorisation

Si le commit refactorise le code existant (il renomme une variable, modifie la portée de la variable, modifie l’empreinte d’une fonction en ajoutant ou supprimant des arguments, ou supprime quelque chose), demandez-vous :

1. Cela peut-il être supprimé ? Cela affectera-t-il la branche stable?
2. Toutes les occurrences sont-elles supprimées ?

Vous pouvez utiliser la commande grep pour le savoir. Vous ne croiriez pas combien de fois j’ai voté -1 juste à cause de ça. C’est une simple erreur que n’importe qui peut faire, mais cela signifie aussi que n’importe qui peut la découvrir.

Le propriétaire du commit peut facilement ignorer ces choses, ce qui est tout à fait compréhensible. Ça m’est arrivé plusieurs fois aussi. J’avais enfin compris la racine du problème que j’avais résolu, alors j’étais pressé de proposer la révision, puis j’ai oublié de vérifier l’ensemble du référentiel.

Outre le référentiel du projet, il est parfois également nécessaire de vérifier d’autres consommateurs de code. Si un autre projet importe celui-ci, il peut également avoir besoin d’être refactorisé. Dans la communauté OpenStack, nous avons un outil qui recherche dans chaque projet de la communauté.

10. La documentation du projet doit-elle être modifiée ?

Encore une fois, vous pouvez utiliser le commande grep pour vérifier si la documentation du projet mentionne quoi que ce soit en rapport avec le changement de code. Faites preuve de bon sens pour déterminer si un changement doit être documenté pour les utilisateurs finaux ou s’il s’agit simplement d’un changement interne qui n’affecte pas l’expérience utilisateur.

Conseil bonus : soyez prévenant

Soyez prévenant, précis et descriptif si vous faites une suggestion ou un commentaire sur quelque chose après avoir examiné le nouveau code. Posez des questions si vous ne comprenez pas quelque chose. Si vous pensez que le code est erroné, expliquez pourquoi vous pensez ainsi. N’oubliez pas que l’auteur ne peut pas le réparer s’il ne sait pas ce qui est cassé.

Derniers mots

La seule mauvaise critique est l’absence de critique. En examinant et en votant, vous donnez votre point de vue et votez uniquement pour cela. Personne ne s’attend à ce que vous donniez le oui ou le non final (sauf si vous êtes un responsable principal !), mais le système de vote vous permet de donner votre point de vue et votre opinion. Un propriétaire de patch sera content que vous l’ayez fait, croyez-moi.

Pouvez-vous penser à d’autres étapes pour une bonne révision ? Avez-vous une technique particulière différente de la mienne ? Faites-le nous savoir dans les commentaires !