Come gestire un TODO in una richiesta pull?

24

Quando rivedo i cambiamenti in una richiesta di pull, a volte mi imbatto in un commento con una nota "TODO" che potrebbe essere lì per motivi diversi, nel nostro caso principalmente a causa di:

  • la soluzione utilizzata per risolvere un problema può essere migliorata ma richiederebbe molto più tempo per l'investimento. Un autore ha scelto una soluzione più rapida, ma ha messo un commento sul fatto che un'opzione migliore è potenzialmente disponibile
  • c'è un codice temporaneo per risolvere un bug esistente che dovrebbe essere risolto presto

Sapendo che TODO s di solito rimangono nella base di codice per la durata della base di codice, come dovrei reagire a loro in una richiesta di pull? Come posso cortesemente chiedere di evitarlo, o se è davvero giustificato, come posso assicurarmi che l'autore del PR lo seguirà più tardi in futuro?

    
posta alecxe 13.12.2017 - 05:33
fonte

4 risposte

24

Quando dici che "generalmente rimangono nella base di codice per la durata della base di codici" nel tuo team / dipartimento / organizzazione, considera quanto segue:

  • Annota nel DoD che TODO , FIXME o tag simili dovrebbe essere evitato.
  • Utilizza uno strumento di analisi del codice statico come SonarQube per contrassegnare automaticamente la generazione instabile.
  • Permetti loro temporaneamente se, e solo se, c'è un ticket corrispondente nel tuo tracker di problemi. Quindi, il codice può apparire come TODO [ID-123] Description ...

Come menzionato nel mio commento , l'ultima affermazione probabilmente ha senso solo in un ambiente che non lascia marcire i ticket (ad esempio se segui un criterio zero-bug ).

Personalmente, penso che TODO s sia a volte ragionevole, ma non bisogna usarli eccessivamente. Tratto da di Robert C. Martin "Codice pulito: un manuale di abilità software agile" (pagina 59):

TODOs are jobs that the programmer thinks should be done, but for some reason can't do at the moment. It might be a reminder to delete a deprecated feature or a plea for someone else to look at a problem. It might be a request for someone else to think of a better name or a reminder to make a change that is dependent on a planned event. Whatever else a TODO might be, it is not an excuse to leave bad code in the system.

    
risposta data 13.12.2017 - 08:23
fonte
5

I sé di TODO non sono cattivi. Sono un grande fan di non andare mai più di un livello e di correggere sempre 1 numero per ticket tracker.

Uso spesso TODO o FIXME come un modo per ricordare a me stesso che avevo bisogno o voluto tornare per risolvere un problema.

Ad esempio, potrei chiamare add (a, b) e aggiungere un TODO al refactoring del metodo add. Non voglio lavorare sul metodo di aggiunta ora, ma voglio tornare ad esso.

Quindi in una richiesta di pull vedrai TODO o FIXME. Ad esempio, io uso FIXME per allertare altri sviluppatori di aree di codice su cui hanno la responsabilità, con cui non dovrei scherzare. Ad esempio, FIXME add non può accettare numeri negativi.

Per aggirare il problema di non essere visti o ignorati, uso uno script che elenca tutte le linee TODO FIXME e DEGUG. E questo viene aggiunto al messaggio di commit.

È difficile ignorare un messaggio di commit di 400 righe che è tutto TODO. Quindi la gente li aggiusta, sia mentre sta già toccando il codice in questione, sia creando nuovi ticket e correggendo il codice problema da solo.

Per essere onesti, mi assicuro anche che ogni progetto abbia costruito nel tempo di pulizia del codice. Sì be può essere pronto per il lancio il 15, ma stavano facendo il clean-up del codice dal 15 al 30. (sembra strano, ma se hai mai gestito un prodotto, sai che se provi a fare in modo che le attività con bassa visibilità vengano avviate, non ti sarà mai permesso di raggiungerle. Qualcun altro avrà priorità.)

    
risposta data 16.12.2017 - 18:49
fonte
1

Accadrà che ci sono richieste pull che non sono perfette. In questo momento sto facendo del lavoro che può essere fatto "abbastanza bene" nel tempo disponibile, ma non perfetto. Quindi invio una richiesta di pull, inserisco TODO con i commenti nei punti giusti del codice e allo stesso tempo aggiungo un'altra richiesta di modifica per cambiare le cose da "abbastanza buono" a "perfetto".

Questa nuova richiesta di modifica può quindi avere la priorità e verrà gestita quando si tratta della voce con la priorità più alta. Se si decide che deve essere perfetto adesso , allora sarà la prossima cosa che verrà consegnata.

Ora la tua domanda: "Come posso cortesemente chiedere di evitarlo, o se è davvero giustificato, come posso assicurarmi che l'autore del PR lo seguirà più tardi in futuro?" Con quello che descrivo, sembra essere piuttosto stupido. Se ho la scelta tra la spedizione in ritardo e la spedizione di ciò che è abbastanza buono, allora non è una tua decisione da prendere. Puoi chiedere al tuo product manager se lo desideri. E "assicurati che l'autore del PR lo seguirebbe"? Se hai una squadra, allora dovresti assicurarti che questo sia registrato nei tuoi sistemi, e speriamo che il tuo team sia organizzato abbastanza bene da non perdere le cose registrate, e qualcuno lo risolverà alla fine, quando non ci sono articoli con priorità più alta. Ricorda, è uno sforzo di squadra.

    
risposta data 17.12.2017 - 14:49
fonte
1

Se il tuo progetto tiene traccia degli elementi in sospeso nel codice sorgente con i commenti di TODO , devi permetterlo.

Il fatto che la presenza di un commento di TODO nella richiesta pull sia un bug, dovresti dirti che tenere traccia degli elementi in sospeso nel codice sorgente è una cattiva idea. Le cose tendono a perdersi o ignorate in questo modo. Ora, se stai parlando di una richiesta di pull a un "fork di lavoro", la situazione è diversa. Un "fork di lavoro" è proprio questo: un work in progress. Ma una forcella come quella di solito non richiede una richiesta di pull. Le "Regole della casa" qui suggerite sono per le richieste di pull per il ramo master .

House Rule # 1 - Tutti i commit su master dovrebbero essere pronti per essere testati, dato che master viene costruito ogni giorno dopo ogni check-in. Tali commit dovrebbero includere anche eventuali test aggiuntivi richiesti.

Se il commento TODO è presente perché il codice non è finito, oi test non sono terminati, o il codice non è in alcun modo pronto per il test, allora quel codice appartiene a un commit locale, non a un richiesta di pull. Chiamami quando è pronto.

Regola n. 2: tutte le informazioni relative ai problemi aperti sono memorizzate nel tracker dei problemi. Tutto. Note, scarabocchi, intuizioni, qualsiasi cosa.

Se il commento TODO riguarda un problema aperto e non è una correzione effettiva per quel problema, allora tali informazioni appartengono al tracker dei problemi. In questo modo, prima che un problema venga chiuso, tutte le informazioni possono essere riviste e verificate, se necessario, per assicurarti che il problema sia effettivamente risolto.

Regola n. 3: tutte le informazioni relative alle attività in sospeso appartengono alla coda di priorità (o qualunque sia il nome del sistema).

Per chiarimenti, un'attività in progetto in sospeso è qualcosa che deve essere fatto nel progetto che ha una priorità prestabilita, che si tratti di un difetto scoperto prima della generazione di un ticket di emissione o dell'implementazione di un requisito di progettazione specifico, o uno dei componenti necessari di tale requisito.

Se il commento di TODO è lì per dire che il nuovo codice avrà un impatto su un'attività in sospeso, o per indicare qualcos'altro nella base di codice che ha bisogno di essere guardato durante l'implementazione del nuovo codice, allora quella informazione appartiene a la coda di priorità, sull'attività esistente o come nuova.

Regola n. 4: i suggerimenti appartengono all'Idea Box (o qualsiasi altra cosa).

Se il commento di TODO suggerisce un cambiamento nella progettazione o nell'implementazione del software, allora quell'informazione appartiene alla casella idea del progetto, o "vNext", o "Design Notes", o qualsiasi altra cosa tu abbia per quel tipo di cosa.

La regola di casa n. 5 - i commenti di TODO non sono consentiti nel codice sorgente. PERIODO.

Se ti attieni a questa regola, non dovrai preoccuparti di nessuno che risponda a qualcosa.

    
risposta data 21.12.2017 - 10:32
fonte