Wie gehe ich mit einem TODO in einer Pull-Anfrage um?

24

Wenn ich die Änderungen in einer Pull-Anfrage überprüfe, stoße ich manchmal auf einen Kommentar mit einer "TODO" -Notiz, die aus verschiedenen Gründen vorhanden sein kann, in unserem Fall hauptsächlich wegen:

  • Die Lösung, die zur Lösung eines Problems verwendet wird, kann verbessert werden, würde jedoch erheblich mehr Zeitaufwand erfordern. Ein Autor entschied sich für eine schnellere Lösung, gab jedoch an, dass möglicherweise eine bessere Option verfügbar ist
  • Es gibt einen temporären Code zur Umgehung eines vorhandenen Fehlers, der bald behoben werden sollte

TODOWie soll ich in einer Pull-Anfrage auf sie reagieren, wenn ich weiß, dass sie im Allgemeinen während der gesamten Lebensdauer der Codebasis in der Codebasis verbleiben? Wie kann ich höflich darum bitten, es zu vermeiden, oder wenn es wirklich gerechtfertigt ist, wie kann ich sicherstellen, dass der PR-Autor es später in der Zukunft nachverfolgt?

Alecxe
quelle
Wenn rückgängig gemachte TODOs für Ihr Team zu einem Problem werden, können Sie dann nicht jemanden beauftragen (oder, falls Ihnen diese Befugnis fehlt, Ihren Chef um die Beauftragung eines Mitarbeiters bitten), einige Zeit damit zu verbringen, Ihren gesamten Code durchzuarbeiten und alle TODOs durchzuführen?
nick012000
Ein wichtiger Faktor ist, ob das betreffende TODO von einer Art ist, die von anderen Entwicklern als dem Autor gelöst werden kann. Ein weiterer Faktor ist die pragmatische Einschätzung des Risikos oder der Relevanz dieses TODO - ist es nur ein weinender Wolf?
Rwong
Es ist überhaupt kein Problem, ein paar Aufgaben in Ihrer Codebasis zu haben.
Oliver Watkins

Antworten:

26

Wenn Sie in Ihrem Team / Ihrer Abteilung / Organisation sagen, dass sie "im Allgemeinen für die gesamte Lebensdauer der Codebasis in der Codebasis verbleiben", sollten Sie Folgendes berücksichtigen:

  • Schreiben Sie es in Ihrem unten DoD , die TODO, FIXMEoder ähnliche Tags sollten vermieden werden.
  • Verwenden Sie ein statisches Code-Analysetool wie SonarQube , um den Build automatisch als instabil zu markieren.
  • Lassen Sie sie vorübergehend zu, wenn sich in Ihrem Issue-Tracker ein entsprechendes Ticket befindet. Dann könnte der Code so aussehenTODO [ID-123] Description ...

Wie in meinem Kommentar erwähnt , ist die letzte Aussage wahrscheinlich nur in einer Umgebung sinnvoll, in der Tickets nicht verrotten (z. B. wenn Sie eine Null-Fehler-Richtlinie befolgen ).

Persönlich denke ich, dass TODOs manchmal vernünftig sind, aber man sollte sie nicht übermäßig verwenden. Aus Robert C. Martins "Clean Code: Ein Handbuch für agiles Softwarehandwerk" (S. 59):

TODOs sind Jobs, die der Programmierer für erledigt hält, die er aus irgendeinem Grund derzeit nicht erledigen kann. Dies kann eine Erinnerung daran sein, ein veraltetes Feature zu löschen oder jemanden zu bitten, sich ein Problem anzuschauen. Es kann eine Aufforderung für eine andere Person sein, sich einen besseren Namen auszudenken, oder eine Erinnerung, eine Änderung vorzunehmen, die von einem geplanten Ereignis abhängig ist. Was auch immer es sonst sein TODOmag, es ist keine Entschuldigung, schlechten Code im System zu belassen.

beatngu13
quelle
2
Wird das entsprechende Ticket nicht für immer im Rückstand bleiben? Ich habe gesehen, dass sowohl TODOs als auch Tickets für immer ungelöst geblieben sind.
Dzieciou
5
@dzieciou nicht unbedingt, aber natürlich besteht die Gefahr, dass es auch für immer dort bleibt. Wenn Sie jedoch ein Ticket haben, ist dieses Risiko wahrscheinlich geringer als bei einem reinen Codekommentar. Ich denke, es hängt vom Team / der Abteilung / der Organisation ab, ob dies sinnvoll ist.
13
Wenn Sie eine No-ToDo-Richtlinie haben, lassen Entwickler den ToDo-Kommentar einfach aus dem Code und lassen den fragwürdigen, schnellen und schmutzigen Code in. Schlechte Idee.
RibaldEddie
8
Todos sind eine Form der Kommunikation. Zwischen Entwicklern. Manchmal über weite Strecken. Die Verwendung des Wortes TODO in einem Codekommentar ist nichts anderes als syntaktischer Zucker für ein bestimmtes Anliegen.
RibaldEddie
2
Ich denke, die Erwähnung des Hinzufügens zum Issue Tracker ist hier der Schlüssel. Wenn Sie das tun, kann es sogar vorkommen, dass Leute anfangen, das Problem zu beheben, ohne dass Sie davon erfahren. Ich habe es in einer Organisation gesehen. Probleme wurden aufgegriffen, nur weil die Leute Fehler, Refactor-Code usw. gerne korrigierten. Kann manchmal ziemlich ordentlich sein.
Per Lundberg
5

TODO ist nicht böse. Ich bin ein großer Fan davon, nie mehr als eine Ebene tiefer zu gehen und immer 1 Problem pro Tracker-Ticket zu beheben.

Ich benutze oft TODO oder FIXME, um mich daran zu erinnern, dass ich zurückkehren musste oder wollte, um ein Problem zu beheben.

Zum Beispiel kann ich add (a, b) aufrufen und ein TODO hinzufügen, um die add-Methode umzugestalten. Ich möchte jetzt nicht an der add-Methode arbeiten, aber ich möchte darauf zurückkommen.

In einer Pull-Anfrage sehen Sie also TODO oder FIXME. Ich benutze FIXME zum Beispiel, um andere Entwickler von Codebereichen darauf aufmerksam zu machen, dass sie für etwas verantwortlich sind, mit dem ich mich nicht anlegen sollte. Zum Beispiel kann FIXME add keine negativen Zahlen akzeptieren.

Um das Problem zu umgehen, dass sie nicht gesehen oder ignoriert werden, benutze ich ein Skript, das alle TODO FIXME- und DEGUG-Zeilen auflistet. Und das wird an die Commit-Nachricht angehängt.

Es ist schwer, eine 400-Zeilen-Festschreibungsnachricht zu ignorieren, bei der es sich ausschließlich um TODOs handelt. Die Benutzer können diese Probleme beheben, indem sie den fraglichen Code bereits berühren oder indem sie neue Tickets erstellen und den Problemcode eigenständig beheben.

Um fair zu sein, stelle ich auch sicher, dass in jedem Projekt eine Code-Bereinigungszeit eingebaut ist. Ja, be kann am 15. startbereit sein, hat aber vom 15. bis zum 30. Code aufgeräumt. (Scheint seltsam, aber wenn Sie jemals ein Produkt verwaltet haben, wissen Sie, dass Sie, wenn Sie versuchen, Aufgaben mit geringer Sichtbarkeit vor dem Start auszuführen, niemals auf sie zugreifen können. Etwas anderes gewinnt an Priorität.)

coteyr
quelle
1
Ich bin damit einverstanden, dass TODOs an sich nicht böse sind, aber wenn ich sie oft benutze, würde ich das als Lärm bezeichnen. Ich denke auch, dass eine 400-Zeilen-Commit-Nachricht leicht ignoriert wird, weil die Leute dazu neigen, so viel Text zu überspringen. Darüber hinaus kürzen viele Git / VCS-Frontends (z. B. GitHub) Betreffzeilen, die länger als eine bestimmte Anzahl von Zeichen sind.
Beatngu13
Ja, das ist mein Punkt, bei ungefähr 4-5 Zeilen wollen die Leute es aufräumen. Also fangen sie an, TODO zu machen. 400 Zeilen war eine Übertreibung.
Coteyr
Mich würde interessieren, wie das Skript zum Hinzufügen der TODOs zur Festschreibungsnachricht funktioniert.
Simon
Es gibt einen "commit-msg" -Hook, mit dem Sie sich verbinden können.
Coteyr
1

Es kann vorkommen, dass Pull-Anfragen nicht perfekt sind. Im Moment mache ich einige Arbeiten, die in der verfügbaren Zeit "gut genug" erledigt werden können, aber nicht perfekt sind. Also reiche ich eine Pull-Anfrage ein, setze TODO mit Kommentaren an die richtigen Stellen im Code und füge gleichzeitig eine weitere Change-Anfrage hinzu, um die Dinge von "gut genug" auf "perfekt" zu ändern.

Diese neue Änderungsanforderung kann dann priorisiert werden und wird behandelt, wenn es sich um das Element mit der höchsten Priorität handelt. Wenn entschieden wird, dass es jetzt perfekt sein muss , dann wird es das nächste , was zu übergeben.

Jetzt Ihre Frage: "Wie kann ich höflich darum bitten, es zu vermeiden, oder wenn es wirklich gerechtfertigt ist, wie kann ich sicherstellen, dass der PR-Autor es später in der Zukunft weiterverfolgt?" Bei dem, was ich beschreibe, scheint das ziemlich dumm zu sein. Wenn ich die Wahl zwischen einem verspäteten Versand und einem ausreichend guten Versand habe, ist es nicht Ihre Entscheidung, eine Entscheidung zu treffen. Sie können Ihren Produktmanager danach fragen. Und "Stellen Sie sicher, dass der PR-Autor dem nachgeht"? Wenn Sie ein Team haben, sollten Sie sicherstellen, dass dies in Ihren Systemen protokolliert ist, und hoffentlich ist Ihr Team gut genug organisiert, damit protokollierte Dinge nicht verloren gehen, und es wird irgendwann jemand das Problem beheben, wenn es keine Elemente mit höherer Priorität gibt. Denken Sie daran, es ist eine Teamleistung.

gnasher729
quelle
1

Wenn Ihr Projekt ausstehende Elemente im Quellcode mit verfolgt TODO Kommentaren verfolgt, müssen Sie dies zulassen.

Die Tatsache, dass das Vorhandensein eines TODOKommentars in der Pull-Anfrage lästig ist, sollte Sie darauf hinweisen, dass das Verfolgen ausstehender Elemente im Quellcode eine schlechte Idee ist. Dinge neigen dazu, verloren zu gehen oder auf diese Weise ignoriert zu werden. Wenn es sich um eine Zuganforderung an eine "Arbeitsgabel" handelt, ist die Situation anders. Eine "Arbeitsgabel" ist genau das - eine in Bearbeitung befindliche Arbeit. Für eine solche Gabel ist normalerweise keine Zuganforderung erforderlich. Die hier vorgeschlagenen "Hausregeln" gelten für Pull-Anfragen für die Master- Filiale.

Hausregel Nr. 1 - Alle Commits an den Master sollten zum Testen bereit sein, da der Master täglich nach jedem Check-in erstellt wird. Diese Festschreibungen sollten auch alle zusätzlichen erforderlichen Tests enthalten.

Wenn das TODO Kommentar vorhanden ist, weil der Code nicht fertig ist oder die Tests nicht fertig sind oder der Code in keiner Weise zum Testen bereit ist, gehört dieser Code in ein lokales Commit und nicht in eine Pull-Anforderung. Ruf mich an, wenn es fertig ist.

Hausregel Nr. 2 - Alle Informationen zu offenen Problemen werden im Issue Tracker gespeichert. Alles davon. Notizen, Kritzeleien, Ahnungen, was auch immer.

Wenn sich der TODOKommentar auf ein offenes Problem bezieht und keine tatsächliche Lösung für dieses Problem darstellt, gehören diese Informationen in den Issue-Tracker. Auf diese Weise können vor dem Schließen eines Problems alle Informationen überprüft und bei Bedarf überprüft werden, um sicherzustellen, dass das Problem tatsächlich behoben ist.

Hausregel Nr. 3 - Alle Informationen zu ausstehenden Projektaufgaben gehören in die Prioritätswarteschlange (oder wie auch immer der Name Ihres Systems lautet).

Zur Verdeutlichung muss eine ausstehende Projektaufgabe in dem Projekt mit einer festgelegten Priorität ausgeführt werden, unabhängig davon, ob es sich um einen Fehler handelt, der vor der Erstellung eines Problemtickets entdeckt wurde, oder um die Implementierung einer bestimmten Entwurfsanforderung oder um eine der folgenden die notwendigen Komponenten dieser Anforderung.

Wenn der TODOKommentar besagt, dass sich der neue Code auf eine ausstehende Aufgabe auswirkt, oder wenn Sie auf etwas anderes in der zu untersuchenden Codebasis hinweisen, das bei der Implementierung des neuen Codes festgestellt wurde, gehört diese Information in die Prioritätswarteschlange die bestehende Aufgabe oder als neue.

Hausregel Nr. 4 - Vorschläge gehören in die Ideenbox (oder was auch immer).

Wenn das TODO Kommentar auf eine Änderung im Design oder in der Implementierung der Software hindeutet, gehören diese Informationen in die Ideenbox des Projekts oder zu "vNext" oder "Design Notes" oder was auch immer Sie für solche Dinge haben.

Hausregel Nr. 5 - TODOKommentare sind im Quellcode nicht erlaubt. ZEITRAUM.

Wenn Sie sich an diese Regel halten, müssen Sie sich keine Sorgen machen, dass jemand irgendetwas verfolgt.

Mark Benningfield
quelle