Comment gérer un TODO dans une demande de tirage?

24

Lorsque j'examine les modifications apportées à une demande d'extraction, je tombe parfois sur un commentaire avec une note "TODO" qui peut être présente pour différentes raisons, dans notre cas, principalement à cause de:

  • la solution utilisée pour résoudre un problème peut être améliorée mais nécessiterait beaucoup plus de temps d'investissement. Un auteur a choisi une solution plus rapide mais a commenté qu'une meilleure option est potentiellement disponible
  • il existe un code temporaire pour contourner un bogue existant qui devrait être corrigé bientôt

Sachant que TODOles informations restent généralement dans la base de code pendant toute la durée de vie de la base de code, comment dois-je réagir en cas de pull request? Comment puis-je demander poliment de l'éviter, ou si c'est vraiment justifié, comment puis-je m'assurer que l'auteur du RP en assurera le suivi plus tard dans le futur?

alecxe
la source
Si les TODO défaits deviennent un problème pour votre équipe, ne pourriez-vous pas assigner quelqu'un (ou, si vous ne disposez pas de cette autorité vous-même, demander à votre patron d'affecter quelqu'un) pour passer du temps à parcourir tout votre code et à faire tous les TODO?
nick012000
Un facteur important est de savoir si le TODO en question est de nature à être résolu par des développeurs autres que l'auteur. Un autre facteur consiste à évaluer de manière pragmatique le risque ou la pertinence de ce TODO - est-ce simplement le loup qui pleure?
rwong
Avoir quelques TODO dans votre base de code ne pose aucun problème.
Oliver Watkins le

Réponses:

26

Lorsque vous dites qu'ils "restent généralement dans la base de code pendant toute la durée de vie de la base de code" dans votre équipe / département / organisation, tenez compte des points suivants:

  • Écrivez-le dans votre DoD que TODO, FIXMEou balises similaires doivent être évités.
  • Utilisez un outil d'analyse de code statique tel que SonarQube pour marquer automatiquement la génération instable.
  • Autorisez-les temporairement si et seulement si un ticket correspondant existe dans votre outil de suivi des problèmes. Ensuite, le code peut ressembler àTODO [ID-123] Description ...

Comme mentionné dans mon commentaire , la dernière déclaration n'a probablement de sens que dans un environnement qui ne laisse pas les tickets pourrir (par exemple si vous suivez une politique zéro bug ).

Personnellement, je pense que les TODOs sont parfois raisonnables, mais il ne faut pas les utiliser de manière excessive. Extrait de "Clean Code: A Handbook of Agile Software Craftsmanship" de Robert C. Martin (p. 59):

TODOs sont des travaux qui, selon le programmeur, devraient être effectués, mais pour une raison quelconque, ils ne peuvent pas le faire pour le moment. Cela peut être un rappel pour supprimer une fonctionnalité obsolète ou un appel à une autre personne pour qu'elle examine un problème. Il peut s'agir d'une demande à quelqu'un d'autre de penser à un meilleur nom ou d'un rappel pour effectuer un changement qui dépend d'un événement planifié. Quoi TODOqu'il en soit, ce n'est pas une excuse pour laisser un mauvais code dans le système.

beatngu13
la source
2
Le ticket correspondant ne restera-t-il pas pour toujours dans l'arriéré? J'ai vu des TODO et des tickets non résolus pour toujours.
dzieciou
5
@dzieciou pas nécessairement, mais, bien sûr, il y a un risque qu'il y reste pour toujours aussi. Cependant, si vous avez un ticket, ce risque est probablement plus faible par rapport à un commentaire de code uniquement. Je pense que cela dépend de l'équipe / du département / de l'organisation si cela a du sens.
13
Si vous avez une politique de non-tâche, les développeurs laisseront simplement le commentaire à faire hors du code et laisseront le code discutable rapide et sale. Mauvaise idée.
RibaldEddie
8
Les Todos sont une forme de communication. Entre développeurs. Parfois sur de longues périodes. L'utilisation du mot TODO dans un commentaire de code n'est rien d'autre que du sucre syntaxique pour une préoccupation particulière.
RibaldEddie
2
Je pense que la mention de l'ajouter au suivi des problèmes est essentielle ici. Si vous faites cela, il pourrait même arriver que les gens commencent à le réparer sans que vous le sachiez. J'ai vu cela se produire dans une organisation; les problèmes ont été détectés simplement parce que les gens aimaient corriger les bogues, refactoriser le code, etc. Peut parfois être assez soigné.
Per Lundberg
5

TODO eux-mêmes ne sont pas mauvais. Je suis un grand fan de ne jamais aller plus d'une couche, et de toujours résoudre 1 problème par ticket de suivi.

J'utilise souvent TODO ou FIXME comme un moyen de me rappeler que j'avais besoin ou que je voulais revenir pour résoudre un problème.

Par exemple, je peux appeler add (a, b) et ajouter un TODO pour refactoriser la méthode add. Je ne veux pas travailler sur la méthode add maintenant, mais je veux y revenir.

Ainsi, dans une demande de tirage, vous verrez TODO ou FIXME. J'utilise FIXME par exemple pour alerter les autres développeurs de domaines de code dont ils ont la responsabilité, avec lesquels je ne devrais pas jouer. Par exemple, FIXME add ne peut pas accepter les nombres négatifs.

Pour contourner le problème de ne pas être vu ou ignoré, j'utilise un script qui répertorie toutes les lignes TODO FIXME et DEGUG. Et cela est ajouté au message de validation.

Il est difficile d'ignorer un message de validation de 400 lignes qui est tous TODO. Donc, les gens les corrigent, soit en touchant déjà le code en question, soit en créant de nouveaux tickets et en corrigeant le code du problème de manière autonome.

Pour être juste, je m'assure également que chaque projet dispose d'un temps de nettoyage de code intégré. Oui, be peut être prêt à être lancé le 15, mais nettoyait le code du 15 au 30. (cela semble étrange, mais si vous avez déjà géré un produit, vous savez que si vous essayez d'avoir des tâches de faible visibilité avant le lancement, vous ne serez jamais autorisé à y accéder. Quelque chose d'autre deviendra prioritaire.)

coteyr
la source
1
Je suis d'accord que les TODOs ne sont pas mauvais en soi, mais les utiliser est souvent ce que je considérerais comme du bruit. Je pense également qu'un message de validation de 400 lignes est facilement ignoré car les gens ont tendance à ignorer autant de texte. De plus, de nombreux frontends Git / VCS (par exemple GitHub) tronquent n'importe quelle ligne d'objet plus longtemps qu'un certain nombre de caractères.
beatngu13
Oui, c'est mon point, à environ 4-5 lignes, les gens ont tendance à vouloir le nettoyer. Alors ils commencent à faire TODO. 400 lignes était une exagération.
coteyr
Je serais intéressé de savoir comment fonctionne le script pour ajouter les TODO au message de validation.
Simon
Il existe un hook "commit-msg" avec lequel vous pouvez vous connecter.
coteyr
1

Il arrivera qu'il y a des demandes d'extraction qui ne sont pas parfaites. En ce moment, je fais un travail qui peut être fait "assez bien" dans le temps disponible, mais pas parfait. Donc, je soumets une demande de pull, je mets TODO avec des commentaires aux bons endroits dans le code, et en même temps j'ajoute une autre demande de changement pour changer les choses de "assez bien" en "parfait".

Cette nouvelle demande de modification peut ensuite être priorisée et elle sera traitée lorsqu'il s'agit de l'élément de priorité la plus élevée. S'il est décidé qu'il doit être parfait en ce moment , alors ce sera la prochaine chose à remettre.

Maintenant votre question: "Comment puis-je demander poliment de l'éviter, ou si cela est vraiment justifié, comment puis-je m'assurer que l'auteur du PR le suivra plus tard dans le futur?" Avec ce que je décris, cela semble plutôt idiot. Si j'ai le choix entre l'expédition tardive et l'expédition de ce qui est assez bon, ce n'est pas votre décision. Vous pouvez en parler à votre chef de produit si vous le souhaitez. Et "assurez-vous que l'auteur du PR en assurera le suivi"? Si vous avez une équipe, vous devez vous assurer que cela est enregistré dans vos systèmes, et j'espère que votre équipe est suffisamment bien organisée pour que les choses enregistrées ne se perdent pas, et quelqu'un le réparera éventuellement, lorsqu'il n'y a pas d'éléments de priorité plus élevée. N'oubliez pas, c'est un effort d'équipe.

gnasher729
la source
1

Si votre projet suit les éléments en attente dans le code source avec des TODOcommentaires, vous devez l'autoriser.

Le fait que la présence d'un TODOcommentaire dans la demande d'extraction soit un bug, vous devez vous dire que le suivi des éléments en attente dans le code source est une mauvaise idée. Les choses ont tendance à se perdre ou à être ignorées de cette façon. Maintenant, si vous parlez d'une demande de tirage vers une «fourchette de travail», la situation est différente. Une «fourchette de travail» n'est que cela - un travail en cours. Mais une fourchette comme celle-ci ne nécessite généralement pas de demande de tirage. Le suggéré « House Rules » sont ici pour les demandes de traction pour le maître branche.

Règle maison n ° 1 - Tous les commits vers master doivent être prêts pour les tests, car master est construit quotidiennement après chaque check-in. Ces commits doivent également inclure tous les tests supplémentaires requis.

Si le TODOcommentaire est là parce que le code n'est pas terminé, ou que les tests ne sont pas terminés, ou que le code n'est en aucun cas prêt pour les tests, alors ce code appartient à une validation locale, pas à une demande de tirage. Appelez-moi quand c'est prêt.

House Rule # 2 - Toutes les informations concernant les problèmes en suspens sont stockées dans le suivi des problèmes. Tout. Notes, gribouillis, intuitions, peu importe.

Si le TODOcommentaire se rapporte à un problème ouvert et n'est pas un correctif réel pour ce problème, alors ces informations appartiennent au suivi des problèmes. De cette façon, avant la fermeture d'un problème, toutes les informations peuvent être examinées et vérifiées, si nécessaire, pour vous assurer que le problème est réellement résolu.

House Rule # 3 - Toutes les informations concernant les tâches de projet en attente appartiennent à la file d'attente prioritaire (ou quel que soit le nom de votre système).

Pour plus de précision, une tâche de projet en attente est quelque chose qui doit être fait dans le projet qui a une priorité définie, qu'il s'agisse d'un défaut qui a été découvert avant qu'il ne génère un ticket de problème, ou la mise en œuvre d'une exigence de conception spécifique, ou l'une des composants nécessaires de cette exigence.

Si le TODOcommentaire est là pour dire que le nouveau code aura un impact sur une tâche en attente, ou pour signaler quelque chose d'autre dans la base de code qui doit être examiné et qui a été découvert lors de l'implémentation du nouveau code, alors ces informations appartiennent à la file d'attente prioritaire, soit sur la tâche existante ou comme nouvelle.

Règle n ° 4 de la maison - Les suggestions appartiennent à la boîte à idées (ou autre).

Si le TODOcommentaire suggère un changement dans la conception ou la mise en œuvre du logiciel, alors cette information appartient à la boîte à idées du projet, ou "vNext", ou "Design Notes", ou tout ce que vous avez pour ce genre de chose.

House Rule # 5 - les TODOcommentaires ne sont pas autorisés dans le code source. PÉRIODE.

Si vous vous en tenez à cette règle, vous n'aurez pas à vous soucier du suivi de quoi que ce soit.

Mark Benningfield
la source