Ce qui fait une bonne pull request dans une codebase Ruby on Rails

La qualité d'une revue de code tient moins à l'attention des relecteurs qu'à la manière dont la pull request a été construite dès le départ. Voici ce que nous regardons, et ce que nous demandons aux ingénieurs de modifier, lorsque nous rejoignons une équipe Rails en tant que contributeur de niveau staff.

Assez petite pour tenir dans la tête

Une pull request qui touche une douzaine de fichiers pour trois sujets sans rapport oblige le relecteur à la valider les yeux fermés ou à passer une heure à reconstituer l'intention. Nous visons des changements qui font une seule chose, et nous séparons les refactorings des changements de comportement dans des PR distinctes, même quand la tentation de les regrouper est forte.

Une description qui répond à « pourquoi », pas seulement à « quoi »

Le diff montre déjà ce qui a changé. Ce qu'il ne peut pas montrer, c'est pourquoi cette approche plutôt que l'alternative évidente, ce qui a été essayé puis écarté, et sur quoi le relecteur doit porter le plus d'attention. Une description de deux lignes qui dit « corrige un bug » ne donne au relecteur rien à vérifier.

Des tests qui auraient échoué sur main

La chose la plus utile qu'un relecteur puisse faire avant de lire la moindre ligne d'implémentation est de vérifier que le test ajouté échoue bien sur l'ancien code. Un test qui passe avant comme après le changement ne teste pas le correctif : il teste seulement que le fichier compile.

# Prouvez-le d'abord : annulez temporairement le correctif en local,
# lancez uniquement le nouveau test, vérifiez qu'il échoue, puis réappliquez le correctif.
bundle exec rspec spec/services/invoice_charger_spec.rb

Pas de refactoring au passage

Renommer une variable, reformater un fichier ou « nettoyer » une méthode sans rapport dans une PR de correction de bug triple la surface à vérifier pour le relecteur et rend l'historique de blame inutilisable pour la prochaine personne qui déboguera cette ligne. Si cela vaut la peine d'être fait, cela mérite sa propre PR.

Rien de tout cela ne demande d'outillage. Cela demande de traiter la pull request elle-même comme un livrable, et non comme un simple contenant pour un diff.