Gerrit/Code review/fr

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
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 :



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 de 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 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  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 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 de l'architecture.

Logique

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



Finaliser la relecture


Laisser un commentaire positif

 * If you want to help to review the code, but don't feel comfortable (yet) making the final decision, you can use  in Gerrit and indicate whether you've "verified" and/or "inspected" the code.
 * If the revision is good and has passed all tests above, mark it  in Gerrit. If you are particularly impressed by someone's work, say so in a comment. When you mark a commit with , you're saying:
 * I've inspected this code, and
 * the code makes sense, and
 * the code works and does something that we want to do, and
 * the code doesn't do anything that we don't want it to do, and
 * the code follows our development guidelines, and
 * the code will still make sense in five years.



Laisser un commentaire négatif

 * If the revision is trivial, broken and has no obvious value, mark the commit as "Code-Review -2"
 * If the revision has issues but also has some utility, or is showing signs of heading in the right direction, mark it  and add a comment explaining what is wrong and how to fix it. Never mark something   without adding a comment. If someone marks your code   it means that your code is good, but needs improvement.

You have to weigh up the costs and benefits of each course of action. If you reject the change completely, then that change will be lost, and the developer may be discouraged. If you tolerate the fault, the end product will suffer. If you fix it yourself, then you're letting yourself get distracted, and perhaps you're making the developer believe that it is acceptable to submit low-quality code and then let someone else worry about the details.

General guidelines on comment style, especially when giving negative feedback:
 * 1)  Focus your comments on the code and any objectively-observed behavior, not motivations; for example, don't state or imply assumptions about motivating factors like whether the developer was too lazy or unexperienced to do things right. Ask questions instead of making demands to foster a technical discussion: "What do you think about...?" "Did you consider...?" "Can you clarify...?"
 * 2)  Be empathetic and kind.  Recognize that the developer has probably put a lot of work in their idea, and thank them for their contribution if you feel comfortable and sincere in doing so. "Why didn't you just..." provides a judgement, putting people on the defensive. Be positive.
 * 3)  Let them know where they can appeal your decision.  For example, invite them to discuss the issue on wikitech-l or on IRC.
 * 4)  Be clear.  Don't sugarcoat things so much that the central message is obscured.
 * 5)  Most importantly, give the feedback quickly. Don't just leave negative feedback to someone else or hope they aren't persistent enough to get their contribution accepted.



Voir aussi

 * Gerrit/+2
 * Gerrit/Code review/Getting reviews
 * Git/Reviewers