«Il était une fois une révision du code, en écrivant des commentaires dans le courrier, en indiquant le numéro de ligne. C'était très drôle. Des pros: personne n'a regardé quoi que ce soit sur les diffs, regardé dans l'IDE. Mais il y avait aussi un inconvénient: après une certaine fusion, les numéros de ligne ont changé. "
Alexandre Makarov, Yii
«Notre entreprise a un concept intéressant - une demande de chaise . C'est à ce moment-là, dans le même bureau, qu'un développeur se roule vers vous sur une chaise et vous dit: «Regardez, c'est plus rapide que de créer une pull request.
Anton Morev, WormSoft
Récemment, il y a eu un enregistrement public du podcast SDCast sur la révision de code sur YouTube. Nous avons sélectionné et déchiffré les plus intéressants du numéro.
Version audio complète sur Spotify, Ya Music et sous forme de fichier à télécharger
Version vidéo complète sur Youtube
Ne considérez pas les révisions de code comme la vérification de quelque chose ou la recherche de bogues
Sergey Zhuk, Skyeng: Je pense que l'objectif fondamental d'une révision de code est de partager les connaissances au sein de l'équipe et de trouver la meilleure solution. L'équipe examine les changements proposés - et les connaissances sur le domaine sont uniformément réparties entre ses membres. L'auteur de la solution comprend comment le code qu'il pensait parfait peut réellement être amélioré.
Par conséquent, les révisions de code ne sont pas la chose à éviter ou à faire plus rapidement. C'est un investissement dans l'entreprise et dans l'équipe: le code s'améliore, l'équipe s'améliore. Oui, au début, je n’ai pas aimé quand la demande a été encapsulée (et qui aimerait ça).
Mais ensuite, j'ai commencé à le voir comme un processus d'apprentissage, tout comme la lecture de livres ou la participation à des conférences.
J'ai ressenti cela plus moi-même, en tant que développeur, j'ai grandi avec cette approche.
Mais il y a une nuance. Beaucoup d'entre vous ont sûrement rencontré des demandes de mille lignes, où le refactoring et une fonctionnalité, et quelques modifications mineures ont été mélangées. En mélangeant différentes choses dans une seule demande, nous compliquons à la fois le partage des connaissances et la vie de l'examinateur: il lui sera plus difficile d'évaluer si le comportement du système a changé, si l'exigence métier a été satisfaite, si le problème dans son ensemble a été résolu - et la solution réussit-elle?
Nous avons remarqué ce moment dans notre équipe et introduit une règle: en une seule pull request, ne pas interférer avec des changements de nature différente. Vous envoyez le refactoring séparément, la fonctionnalité séparément et les modifications mineures séparément.
Rapport de Sergey sur les pratiques de son équipe. La version texte est ici .
Ces demandes sont généralement examinées rapidement et facilement et sont beaucoup plus susceptibles de recevoir des commentaires de qualité. Et du côté du mainteneur, il y a des avantages: si vous aimez le refactoring et la fonctionnalité avec un bug, vous pouvez fusionner le refactoring séparément.
Alexander Makarov: Je suis d'accord que vous ne devriez pas essayer de passer le moins de temps possible sur les révisions de code. Ouvert, comme les crochets sont bons, ce code fait quelque chose - il ne fonctionne pas de cette façon. Si le réviseur ne peut pas garantir le code jusqu'à la fin, cela signifie que la révision du code n'a pas été effectuée.
Par conséquent, nous avons maintenant commencé à compiler un assez grand nombre de directives pour nous-mêmes, et l'une d'elles parle de révision du code.
Une partie de la ligne directrice de l'équipe Yii.
Mais à la thèse sur les petites demandes de tirage individuelles: j'ai eu des situations où un prospect est venu et a introduit quelque chose comme ça. L'équipe était hostile. Comment l'as-tu obtenu?
Sergey Zhuk: Il y avait une équipe en parallèle avec laquelle nous avons interagi et utilisé leur API. Ils ont fait un tas de fonctionnalités en un mois, nous en avons fait juste un peu. Autrement dit, au départ, nous ne nous sommes pas concentrés sur la fourniture de fonctionnalités, mais sur la qualité avec cette approche. Et nous avons convenu avec l'entreprise que nous le publierions plus lentement, mais nous essayons de ne rien casser. Un mois plus tard, un des voisins est tombé en panne, puis un autre. Mais nous ne le faisons pas. Et sur cet exemple, tout le monde a tout compris.
Konstantin Burkalev, SDCast:J'avais des processus pour implémenter la révision du code en général - et ce n'était pas facile, même si tout le monde semblait comprendre que c'était bien, oui. Vous dites: "Les gars, fusionnons via une pull request." Ils vous disent: "Oui, il y a une petite particularité."
Le principe fonctionne très bien ici: lorsque quelque chose se brise, vous dites: «Mais si vous aviez fait une demande, nous aurions cherché et cela n’en est pas venu à cela. L'idée est que les gens comprennent les erreurs à partir de leur propre expérience. Essayer d'imposer ne fonctionne pas toujours.
À quelle vitesse examiner
Au cours de l'émission, un vote a eu lieu parmi le public. Voici l'un d'entre eux.
Konstantin Burkalev: Juin est particulièrement souvent comme ceci: «Eh bien, vous avez regardé ma demande, est-ce que ça va? Je l'ai écrit, j'ai serré les poings et j'ai attendu ».
Et j'ai ma propre tâche, je ne peux y arriver que le soir ou je ne sais pas du tout ...
Sergey Zhuk: Dans notre équipe, la révision a toujours été une priorité. Par conséquent, il y a un règlement - quand une pull request arrive, vous terminez ce que vous faites maintenant, pour ne pas perdre le contexte, et passez à la révision. Parce que ce que vous êtes sur le point de faire est toujours en cours. Et cette tâche a déjà été accomplie.
Le code a déjà été écrit, il ne reste plus qu'à le regarder, le fusionner et le télécharger sur le produit.
C'est-à-dire que j'ai terminé quelque chose de moi-même, changé, rapidement regardé et continué à travailler. Probablement, 3 fois par jour, je suis distrait de mes tâches d'examen. Mais, encore une fois, vous devez comprendre que dans mon équipe, tout est divisé en petites demandes de tirage, de 200 à 300 lignes de code chacune. En conséquence, un minimum de temps est passé.
"Qui examine est moins important que qui"
Konstantin Burkalev: Cela soulève la question - qui devrait revoir. Plomb seulement? Seul le sénateur? Ou peut-on et devrait être donné à quelqu'un de moins compétent, juste pour que la personne essaie de grandir?
Et lorsqu'on leur a demandé ce qu'il fallait examiner, les gens ont répondu comme ça.
Alexander Makarov: Si vous avez beaucoup de monde dans votre équipe et que le leader a créé un goulot d'étranglement de lui-même, cela ralentit toute l'équipe et, par conséquent, aggrave l'équipe. En tant que responsable, lorsque je travaillais chez Skyeng, j'avais 15 pull requests par jour à leur apogée, et pas les plus petites. J'ai réservé du temps pour l'examen le matin et le soir.
Il est nécessaire de revoir tout le monde. Sauf, peut-être, pour les histoires où «le feu, tout est en feu» - ça ne va pas devenir pire là-bas.
Je me trompe, ça va - même si j'utilise le même projet depuis de très nombreuses années. Alors maintenant, j'essaie d'inviter autant de personnes que possible à voir ma demande. C'est bien, ça devrait l'être.
C'est une autre question de savoir si tout le monde doit faire confiance à l'examen. J'avais des gars qui pouvaient bien comprendre, mais ils avaient de gros problèmes de concentration: et, disons, l'un d'entre eux a passé 6 heures sur une critique. Je devais apprendre aux gens à gérer leur temps.
Si nous parlons de sociétés commerciales, je suis en faveur d'identifier qui a cette responsabilité et qui peut examiner à volonté - et combien de temps par jour vous pouvez consacrer à l'examen, en fonction de ce statut.
Anton Morev:Comme je le vois, il existe différents processus. Si nous faisons une fonctionnalité qui doit être publiée dans un court laps de temps, nous n'avons pas le temps de laisser Jun voir le code. Mais si nous faisons une sorte de produit interne ou que la date limite n'est pas élevée, oui, c'est une bonne pratique de laisser June examiner ce que le développeur principal ou senior a fait. Ainsi, les Jun comprendront mieux ce qui se passe dans le projet dans son ensemble et comment fonctionne le mécanisme de prise de décision.
"C'est un rejet tout de suite"
Sergey Zhuk: Skyeng a implémenté un contrôle pour un message de validation: il doit y avoir un numéro de tâche dans JIRA, sinon la pull request ne peut pas être fusionnée. Parfois, vous ouvrez le code, vous ne comprenez tout simplement pas ce qu'il y a - vous ouvrez une tâche et vous pouvez comprendre quelque chose.
Sinon, au début, c'était difficile, puis nous avons décidé de le rejeter uniquement si la tâche était complètement mauvaise ou si l'équipe était en désaccord architectural. Et donc: nous mettons une approbation, fusionnons, écrivons un commentaire - et si l'auteur de la pull request le souhaite, il reviendra et le corrigera. Là, il corrigera les petites rugosités ou utilisera une sorte de motif. Quelles sont vos pratiques de rejet?
Alexander Makarov: Les critères coïncident complètement avec les vôtres - si la tâche n'est pas bien faite, bien sûr, vous devez la conclure. Même si le code semble correct et architecturalement tout est cool.
, : , .
Par exemple, nous disons: «Les gars, faisons un test». Il y a des exceptions, bien sûr, mais les tests sont très importants. Il est clair d'après eux si la personne a bien compris la tâche: encore une fois, s'il teste des classes et des méthodes, et non un cas d'utilisation, c'est déjà suspect. Nous avons maintenant foiré l' infection , c'est un test de mutation. Tulza laisse les mêmes tests, mais modifie le code et s'exécute. Et si le code modifié passe avec les mêmes tests, une partie du code n'est pas nécessaire - vous pouvez simplement la prendre, la supprimer, rien ne changera.
Un peu de backstage.
Aussi, bien sûr, des problèmes de sécurité et de performance - il y aura un rejet ici. Nous ne rejetons pas les bagatelles: soit nous demandons de les réparer, soit nous les réparons nous-mêmes - nous poussons directement dans les pull requests de ceux qui les ont faites, et nous montrons déjà sur le code fini quoi, comment et pourquoi nous l'avons fait.
Anton Morev:Concernant ce que vous avez dit, le problème est-il résolu? Il arrive qu'un développeur, en résolvant un problème, en ait résolu un autre. Il n'est pas nécessaire de rejeter de telles situations. Ou du moins comprendre comment la tâche a été modérée.
Konstantin Burkalev: Mais le moment de lier les messages de validation à un tracker de tâches me semble important. Il y a des tâches quotidiennes dans lesquelles cela aide beaucoup. Savez-vous quand: "Écoutez, nous avons fait quelque chose de similaire dans le problème du bouton." Vous trouvez la tâche sur le bouton, comprenez le numéro, allez dans le journal du référentiel, trouvez le diff de ces commits et voyez: en effet, nous avons vissé ceci et cela, il peut être déplacé ici.
Mais le contraire se produit également. Je regarde le code source d'un projet ici et je tombe sur la fonction action684 dans le backend.
J'écris à un ami, je demande, quel est ce nom cool dans le code. Il répond - une référence à la tâche dans le tracker. "Pourquoi trouver un nom pour une fonction, vous l'écrivez dans le cadre d'une tâche" ... Thresh, qui ne devrait pas exister dans un monde normal, bien sûr.