Project

General

Profile

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. À ce sujet ça peut sembler ridicule de pointer des problèmes de style dès le premier moment mais c'est surtout ridicule parce que de tels problèmes ne devraient pas arriver avec un environnement de développement adéquat, qui indente de manière uniforme par exemple. En pratique l'utilisation de black est mise en place dans Chrono, pourrait être généralisée une fois intégrée dans jenkins etc.

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: avoir un screenshot avant (par l'auteur du ticket) et après (par le responsable du ticket) permettant une relecture facile.

Exceptions

  • tabellio.*, themis.*, pfwbged.* et les autres modules Plone; on doit faire là avec un historique de code et technos mal partagé, Frédéric y est tout seul :/

Ressources

Also available in: PDF HTML TXT