Gerrit/Relecture de code

Ceci est un guide pour la relecture et la fusion (merge) des contributions aux dépôts de code Wikimedia, écrit principalement pour les développeurs assurant la fonction de relecteur de code.

• Pour les développeurs qui soumettent leur code, voir Faire relire son propre code.
• Pour une introduction à l'outil Gerrit (commenter le code; comparer les ensembles de correction), voir le Tutoriel Gerrit.

Objectifs

• Fournissez des critiques rapides pour éviter la lente détérioration des performances et de l'intégrité des données stockées et les sentiments d'abandon. Les retours rapides encouragent les développeurs à continuer à participer et contribuent ainsi à élargir notre base de collaborateurs.
• Soyez gentil. Les corrections des contributeurs sont des dons. Les relectures de code influencent la perception qu'a un bénévole à propos du projet entier.
• A long terme, encouragez les contributeurs du code à devenir également des relecteurs.

Chercher des corrections à relire

Certains développeurs peuvent vous demander à l'aide d'un drone, de relire leur code.

Il y a beaucoup de code à relire. Comment décomposer la tâche en sous-tâches plus gérables ?

Il existe plusieurs modèles de base :

A la demande

Contributors can request code review in Gerrit by adding a reviewer and clicking "Add to attention set". This is a helpful way to indicate to a user that you wish them to do a review.

In the Gerrit UI patch contributors can request reviewer by adding their user name and clicking "Add to attention set"

Reviewers should therefore bookmark and monitor requests for code review in this form using the Gerrit URL query has:attention.

When a patch has your attention you can either:

• Provide a review for the patch as requested (which will move the attention back to the patch contributor) OR
• If you are not the right person to review the patch, comment and/or suggest alternative reviewers using the "add to attention set" feature. You can use the Maintainers page to identify alternative reviewers.

Par projet

Si vous êtes un mainteneur de code, pensez à déclarer les notifications par courriel pour les nouvelles corrections dans vos projets (les dépôts) via les Projets suivis dans Gerrit. Vous pouvez aussi vous inscrire auprès du Robot relecteur de Gerrit qui va vous ajouter automatiquement en tant que relecteur pour chaque nouvelle correction.

• Identifiez les principales parties du travail ou les ensembles de relectures associées. Une relecture rapide et chronologique peut être un bon moyen pour faire cela; mais vous pouvez aussi choisir un dépôt avec de nombreuses corrections ouvertes.
• Ouvrez toutes les pages de correction dans les onglets différents d'une fenêtre de votre navigateur. Ouvrez les fichiers concernés dans un éditeur de texte.
• Relisez dans leur intégrité les nouveaux fichiers ainsi que ceux qui ne vous sont pas familiers. Choisissez un ensemble de corrections avec une modification importante dans les fichiers associés et relisez les.

Par auteur

• Choisissez un auteur avec (beaucoup) de corrections ouvertes, chargez-les dans Gerrit.
• Parcourez les versions chronologiquement, ou traitez un sujet ou un répertoire à la fois.
• Cette méthode permet au relecteur de connaître les développeurs individuellement : leurs compétences, leurs défauts et leurs intérêts. Le travail comporte une idée de progression et de continuité.

Si quelqu'un vous a déja ajouté en tant que relecteur potentiel et que vous savez que vous ne pourrez pas relire la correction, retirez simplement votre nom de la liste des relecteurs.

Nouveaux contributeurs de nos projets

Vous pouvez ajouter (certaines) des requêtes ci-dessous à votre menu en modifiant vos préférences utilisateur. Un nouveau contributeur est défini comme une personne qui a fourni de une à cinq modifications au total.

• Ensembles de modifications ouvertes, faites par les nouveaux contributeurs, vérifiées avec succès par le robot Jenkins et en attente des commentaires des relecteurs
  • Relisez la correction si vous êtes familier avec le dépôt et donnez votre verdict (CR±1 ou CR±2).
• Ensembles de modifications ouvertes, faites par les nouveaux contributeurs, vérifiées avec succès par le robot Jenkins et approuvées par un relecteur (CR>0)
  • Aidez à prendre une décision (CR±1, ou CR-2, ou CR+2 si vous avez les droits +2 sur le dépôt).
• Ensembles de modifications ouvertes, faites par les nouveaux contributeurs, vérifiées avec succès par le robot Jenkins mais rejetées par un relecteur (CR<0)
  • Demandez si le contributeur a besoin d'aide et s'il a l'intention de continuer à améliorer sa correction.
• Ensembles de modifications ouvertes, faites par les nouveaux contributeurs, et en échec lors de leur vérification par le robot Jenkins
  • Demandez si le contributeur a besoin d'aide et s'il a l'intention de continuer à améliorer sa correction.
• Les résultats des demandes ci-dessus sont tous inclus dans cette unique requête : Toutes les corrections ouvertes des nouveaux contributeurs

Ordre chronologique (et chronologique inverse)

• Commencez par la correction ouverte la plus ancienne, relisez en progressant jusqu'à la fin de la liste. Alternativement, commencez par la dernière version et relisez chacun des diff à son tour jusqu'au dernier. Cette approche est acceptable pour des versions mineures, mais nécessite de basculer régulièrement entre les projets et leur contexte.

Liste des points de relecture à vérifier

Réellement souhaité ?

• La toute première question est de savoir si la contribution est une bonne idée. Si la contribution est inutile ou ne suit pas la direction et le but du projet, expliquez et fournissez vos commentaires avec de meilleures idées.

Général

• Le code fourni par les contributeurs doit fonctionner comme indiqué, c'est à dire, que toute faute trouvée dans le code doit être corrigée. (Prenez garde à ne pas blâmer le développeur actuel pour un code qui aurait été écrit par son prédécesseur.)
• Maintenez la compatibilité arrière pour les interfaces stables si cela est relativement simple à faire.
• Si une modification proposant une rupture est nécessaire pour faire des changements significatifs, assurez-vous de suivre la Politique des interfaces stables.
• Lire les rapports de bogue associés ou la documentation.
• Tenez-vous au courant des problèmes techniques importants. Lisez les spécifications associées ou les sections des manuels.

Performance

• Le code exécuté plusieurs fois dans une requête, ainsi que le code qui est exécuté au démarrage doit être relu pour des raisons de performance (par exemple par un membre de la Wikimedia Performance Team). Tous code douteux doit être passé au banc d'essais.
• Tout code accessible à partir du web et peu efficace (en temps, en mémoire, en nombre de requêtes réalisées, etc.) doit être marqué pour être corrigé (par exemple en créant une tâche pour l'Equipe Performance dans Phabricator).
• Les modifications du schéma de la base de données ou les modifications des requêtes à fort traffic doivent être relues par un expert de la base de données. (Une tâche correspondante Phabricator doit avoir la marque schema-change associée.)

Conception

• Est-ce que cette modification améliore ou fait régresser l'expérience de l'utilisateur final ? Si elle a un impact sur l'expérience utilisateur ou sur la conception visuelle, consultez #wikimedia-design ^connecter ou la liste de diffusion de l'architecture, ou l'un des mainteneurs de l'architecture.

Style

• Assurez-vous que les conventions de style du code sont respectées, telles que l'espace, la longueur des lignes, la position des accolades, etc.

Lisibilité

• Les fonctions doivent faire ce que leur nom indique. Le choix du verbe correct est essentiel, une fonction get*() doit récupérer quelque chose, une fonction set*() doit initialiser quelque chose.
• Les noms de variables doivent être en anglais, et les abbréviations doivent être évitées autant que possible.
• On préfèrera généralement les commentaires documentant les fonctions.
• Tout code apparaissant compliqué doit être simplifié. Ce qui en augmente la taille. Par exemple :
  • Les opérateurs ternaires (?:) pourraient être remplacés par 'if / else'.
  • Les longues expressions pourraient être découpées en plusieurs instructions.
  • L'utilisation astucieuse de la précédence des opérateurs, de l'évaluation des raccourcis, des expressions d'assignation, etc. pourrait être réécrite.
• La recopie du code doit être évitée, qu'elle soit faite à l'intérieur d'un même fichier, ou entre fichiers.
  • Cela nuit à la lisibilité car cela oblige plutôt le lecteur à rechercher ce qui a bien pu changer. La relecture de copies de code presque semblables prend forcément plus de temps que celle d'une seule instance.
  • Cela nuit également à la maintenance car si l'erreur (ou la fonctionnalité absente) se trouve dans la partie copiée, alors ce sont toutes les intances qui doivent être corrigées.
  • Certains développeurs peuvent recopier de grandes sections de code à partir d'autres extensions ou à partir du noyau, et y modifier quelques détails à l'intérieur. Si un développeur semble avoir écrit du code trop bon en rapport avec son niveau d'expérience, essayez de faire un grep sur la base de code avec quelques fragments afin d'identifer la source. Orienter le développeur vers soit la réécriture du code, soit sa restructuration.
  • Utiliser des raccourcis peut être contre-productif, car le temps passé à créer le raccourci et vérifier qu'il fonctionne aurait pu être utilisé pleinement pour développer simplement l'idée initiale.

Sécurité

• Le relecteur doit avoir lu et compris le Guide de la sécurité et être conscient des problèmes de sécurité qui y sont présentés.
• Il ne doit y avoir aucune possibilité d'exécuter à distance du code arbitraire sur le serveur. Cela signifie qu'il ne doit y avoir aucun eval() ni create_function(), ni modificateur /e sur preg_replace().
• Vérifiez les problèmes d'injection de protocol textuel (XSS, injection de SQL, etc.) Insistez sur l'échappement des données en sortie.
• Vérifiez toutes les actions d'écriture pour CSRF.
• Changez les points d'entrée spéciaux pouvant contourner les règles de sécurité dans WebStart.php.
• Changez la duplication inutile de la fonctionnalité noyau MediaWiki relative à la sécurité, comme l'utilisation de $_REQUEST à la place de $wgRequest, ou l'échappement SQL avec addslashes() au lieu de $dbr->addQuotes().
• Uniquement si vous travaillez avec du code ancien : vérifiez les problèmes avec [$1 register_globals], particulièrement les vulnérabilités des inclusions arbitraires classiques. (Register Globals a été supprimé en PHP 5.4.0 et MediaWiki ≥1.27 nécessite PHP ≥5.5.9.)
• En cas de doute, n'hésitez pas à contacter la Wikimedia Security Team.

Architecture

• Les noms appartenant à un espace de noms partagé (global) doivent être préfixés (ou autrement être spécifiques à l'extension en question) pour éviter les conflits entre extensions. Cela comprend :
  • les variables globales
  • les fonctions globales
  • les noms de classes
  • les noms de messages
  • les noms de tables
• Le but de la modularité est de séparer les problèmes. Les modules ne doivent pas dépendre les uns des autres d'une manière inattendue ou complexe. Les interfaces entre les modules doivent être aussi simples que possible sans obliger à dupliquer le code.
• Vérifiez la conformité aux Principes d'architecture.

Logique

• Décrivez les raccourcis et demandez au développeur de réaliser un travail plus propre.

Finaliser la relecture

Avertissement :

Veillez à ne pas utiliser Verified +2. Il s'agit d'une fusion forcée qui saute les tests. Voir aussi les Règles pour fusionner sans relecture de code

Laisser un commentaire positif

• Si vous voulez aider en relisant le code et que vous ne vous sentez pas (encore) prêt à publier votre décision finale, vous pouvez utiliser Code-Review +1 dans Gerrit et indiquer que vous avez vérifié et / ou inspecté le code.
• Si la version est bonne et qu'elle a passé tous les tests ci-dessus, marquez-la Code-Review +2 dans Gerrit. Si vous être particulièrement impressionné par le travail de quelqu'un, indiquez-le dans un commentaire. Lorsque vous marquez une validation avec Code-Review +2, vous voulez dire :
  • J'ai inspecté ce code, et
  • le code a un sens, et
  • le code est fonctionnel et réalise ce que nous voulons, et
  • ce code ne réalise pas ce que nous voulons qu'il ne fasse pas, et
  • le code suit nos règles de développement, et
  • le code aura encore un sens dans cinq ans.

Laisser un commentaire négatif

• Si la version est triviale, cassée et sans valeur évidente, marquez le commentaire comme Code-Review -2
• Si la version présente des problèmes mais qu'elle reste utile, ou qu'elle montre des signes de pouvoir avancer dans la bonne direction, marquez-la Code-Review -1 en ajoutant un commentaire expliquant ce qui ne va pas et comment on pourrait la corriger. Ne marquez jamais Code-Review -1 sans lui ajouter un commentaire. Si vous recevez Code-Review -1 pour votre code cela signifie qu'il est valable mais qu'il doit être amélioré.

Vous devez peser les coûts et les bénéfices de chaque moyen d'action. Si vous rejetez complètement la modification (Code-Review -2), alors celle-ci sera perdue et cela peut décourager le développeur. Si vous tolérez la faute, c'est le produit final qui va en souffrir. Si vous corrigez vous-même, vous avez été distrait et même il est possible que vous avez permis au développeur de penser qu'il est acceptable de soumettre du code de qualité médiocre et de laisser une autre personne s'occuper des détails.

Règles générales sur le style des commentaires, particulièrement quand vous formulez un avis négatif :

1. Focalisez vos commmentaires sur le code et quelques comportements objectifs observés et non pas sur des motivations; par exemple ne déclarez pas ou n'impliquez pas de suppositions sur des facteurs motivants comme la paresse ou l'incompétence du développeur à bien faire les choses. Posez plutôt des questions au lieu de vous occuper d'une discussion technique : « Que pensez-vous si... ? » « Avez-vous pris en compte... ?  » « Pouvez-vous détailler... ?  »
2. Ayez de l'empathie et soyez gentils. Reconnaissez que la personne a probablement investi beaucoup de travail pour développer son idée, et remerciez-la pour sa contribution si c'est sincère et que vous en êtes convaincu. « Pourquoi ne feriez-vous pas... ?  » implique que vous fournissez un jugement, ce qui place les personnes sur la défensive. Soyez positifs.
3. Faites leur savoir où il est possible de faire appel de votre décision. Par exemple invitez les à discuter le problème sur mail:wikitech-l ou sur IRC.
4. Soyez clair. N'enrobez pas les choses à un tel point que l'on ne puisse plus distinguer le message central.
5. Le plus important est de fournir une réponse rapide. Ne laissez pas simplement un commentaire négatif à une autre personne ou n'espérez pas qu'il ne soit plus assez persistant au moment où la proposition sera acceptée.

Contacts

Au cours de la relecture, vous pourriez avoir des questions ou des problèmes. Ne vous inquiétez pas ! On peut essayer de vous aider.

Pour les questions liées à Gerrit Wikimedia et la relecture de code ou des corrections particulières, n'hésitez pas à regarder la liste des canaux IRC et à choisir celui qui vous concerne. Voir aussi la page de Communication pour les plateformes supplémentaires.

Par exemple, pour les questions liées aux corrections MediaWiki, n'hésitez pas à vous joindre à #mediawiki ^connecter.

Si vous avez des problèmes avec le jenkins-bot, n'hésitez pas à contacter #wikimedia-releng ^connecter.

Voir aussi

• Gerrit/+2
• Gerrit/Relecture de code/Ajouter des relecteurs
• Git/Reviewers