HowDoWeDoCodeReviews¶
Parce que ça améliore la qualité du code produit, que ça permet un partage de la connaissance du code, on fait des relectures des patchs avant d'envoyer ceux-ci dans les dépôts. Pour citer "Code reviews: from nitpicking to cooperation" repris dans les ressources en bas de page :
Code review and peer review are great methods for cooperation aiming at:
- Ensuring that the code works as intended
- Ensuring that the task was fully accomplished and no detail left out
- Ensuring that no security issues have been introduced
- Making sure the code fits the practices of the team and is understandable and maintainable by others
- Sharing insights, transferring knowledge between code author and code reviewer
- Helping the code author to learn to write better code
Toutes les applications, tous les modules sont concernés, que ça soit mail2redmine ou w.c.s., et de la même manière, tout le monde est concerné. Comme à tout il peut y avoir des exceptions, elles sont listées en bas de cette page.
Timing et attentes¶
Un auteur n'a pas envie d'attendre des jours que quelqu'un jette un œil à son code; il est souvent utile de préférer une réponse rapide mais incomplète, concentrée sur les points majeurs relevés d'une première lecture, plutôt qu'attendre le moment permettant la relecture intégrale et exhaustive.
Comme les relectures un autre aspect important du développement concerne les tests, de manière graduée une règle peut être qu'un patch doit être couvert par des tests autant que le reste du code du module (cf jenkins pour prendre connaissance de la couverture de code). Avoir les commits poussés dans des branches "wip" permet à jenkins d'assurer l'exécution et la visibilité des tests.
À noter qu'on peut bien sûr également partager des patchs en amont, pour interroger sur une approche, on ne fait pas de test-driven development.
Esprit de relecture¶
L'esprit doit être positif etc. L'article "Code reviews: from nitpicking to cooperation" en ressource pointe bien les risques à exiger des commits "parfaits".
Méthodes particulières¶
- patch CSS: ça peut faciliter la compréhension d'avoir un screenshot avant (idéalement par la personne ayant créé le ticket, sinon par la personne ayant préparé le patch) et après.
Exceptions¶
- Il peut y avoir des changements répétés sur un tas de modules (par exemple pour actualiser un Jenkinsfile), dans ces cas ça peut être ok d’avoir une relecture sur un premier module et qu’ensuite les autres modules soient traités sans relectures spécifiques (dans la mesure où c’est un code déjà validé qui leur est appliqué).
- Les commits de traduction/documentation peuvent également avoir lieu sans relecture.
Divers¶
L'interface de gitea n'aide pas à ça mais c'est bien de jeter un œil à l'écran commit pour lire les intitulés des commits, pour également pouvoir y corriger les erreurs.
Ressources¶