doc: add note about considering making issues over TODO source code comments #18981

kallewoof commented at 7:48 AM on May 15, 2020: member

This PR is in response to #18965 (comment)

fanquake added the label Docs on May 15, 2020

laanwj commented at 9:04 AM on May 15, 2020: member

Thank you. ACK f3370cdcd6a76a2b52b2fc7d2e355e10ce2fb2e4.

in doc/developer-notes.md:164 in f3370cdcd6 outdated

 159 | @@ -160,6 +160,9 @@ A complete list of `@xxx` commands can be found at http://www.doxygen.nl/manual/
 160 |  As Doxygen recognizes the comments by the delimiters (`/**` and `*/` in this case), you don't
 161 |  *need* to provide any commands for a comment to be valid; just a description text is fine.
 162 |  
 163 | +Note: do not add todo or @todo entries in the source code. If something needs to be done, open an
 164 | +Issue instead.

jonatack commented at 9:36 AM on May 15, 2020:

s/Issue/issue

MarcoFalke commented at 9:47 AM on May 15, 2020:

I think a rule without rationale is hard to understand. Maybe give some context and motivation?

MarcoFalke commented at 9:54 AM on May 15, 2020:

Also, it seems too strict and not allow for exceptions. For example if something is out-of-scope for a pull request, but still should be done in the long term, a TODO seems appropriate. Especially when the TODO is clearly motivated and clearly explains how to fix it.

For example:

boost::thread a; // TODO switch to C++11 std::thread, when possible
GetCounter(); // TODO add lock annotation when class has been updated to our annotated mutex (e.g. #17443 (review))
...

The advantage of inline TODO is that when the code gets removed or the TODO gets fixed, it will go away in the same commit. If it was filed as an issue, we'd have to walk the issue list and keep it in sync.

However, for general, non actionable TODOs, an issue makes sense. For example "increase encapsulation" or "increase performance with cache".

Thoughts?

jonatack commented at 10:31 AM on May 15, 2020:

@MarcoFalke heh, you used my review as an example of a good TODO. I guess I should review less and start tackling TODOs :)

laanwj commented at 10:47 AM on May 15, 2020:

Even "switch to std::thread" and "add lock annotations" can very much be issues (an umbrella issue could list the places).

My point is that these kind of long-term goals can change. Maybe lock annotations turn out to be more trouble than they're worth. Maybe std::thread would have turned out to have some problematic behavior on some platform. Code comments in general are prone to becoming stale. (it's not the case, these are fine in retrospect, but an issue allows updating these kind of things without touching the code)

But sure, it's possible to think of exceptions, but I think the general rule should be to not add TODOs in the source code.

kallewoof commented at 10:47 AM on May 15, 2020:

@MarcoFalke Both of those examples serve their purpose better if they are issues, instead of code comments, IMO, but I guess in certain cases (e.g. for things that must wait for something else to happen first) it's OK. Amended.

MarcoFalke commented at 11:00 AM on May 15, 2020:

But sure, it's possible to think of exceptions, but I think the general rule should be to not add TODOs in the source code.

Agree. Concept ACK to have a policy so that TODOs are generally avoided.

jonatack commented at 9:37 AM on May 15, 2020: member

ACK f3370cdcd6a76a2b52b2fc7d2e355e10ce2fb2e4

kallewoof force-pushed on May 15, 2020

kallewoof commented at 10:48 AM on May 15, 2020: member

Updated text to include a rationale and to word things in a less strict fashion, as sometimes TODO entries may be preferred (see #18981 (review)).

MarcoFalke commented at 10:59 AM on May 15, 2020: member

ACK f513b50b7d73565999a4c43e6286ed8ade598c8f seems like a reasonable policy with rationale, while still allowing exceptions

ryanofsky commented at 11:57 AM on May 15, 2020: member

This is grim. We need more todo-style comments, not less. One of the worst thing about our code is that that is is filled with thousands of nonsensical and nonideal things that everybody agrees are nonsensical and nonideal but nobody explains out of laziness or fear of expressing a "developer's opinion". We need comments that talk about opinions and intentions and limitations of code. Code that is intended to be understood by humans should look like code that is written by humans, not drunk mute space aliens. It's great to file issues. But a developer guideline discouraging developers from describing their intentions in code is a WTF to me.

If the main concern here is causal contributors submitting PRs to "fix" todos in ways that aren't useful (which doesn't seem like it's been a problem, but could be a theoretical problem if there are more comments), this just seems like something that could be noted in the contributing guidelines. But I would point out that todo comments can and have lead to useful contributions in the past. Like the ongoing and evolving TODO in interfaces/chain.h in has prompted numerous useful wallet improvements from ariard

ryanofsky commented at 12:07 PM on May 15, 2020: member

I don't think we need a developer guideline about todos, but if we do we do something like google's https://google.github.io/styleguide/cppguide.html#TODO_Comments would be pretty good, imo

kallewoof commented at 4:22 AM on May 16, 2020: member

@ryanofsky I don't think adding TODO entries will do anything about the problems you're listing (nonideal stuff); I think the preference to not do refactors without a cause is what keeps these around indefinitely.

If you look at the Google guidelines you pointed at, the core issue I think is in the last line:

If your TODO is of the form "At a future date do something" make sure that you either include a very specific date ("Fix by November 2005") or a very specific event ("Remove this code when all clients can handle XML responses.").

We are not the kind of organization that can put deadlines on anything, period. I can claim that Signet should be merged by May all I want, but that's not gonna change anything, and Signet isn't gonna go anywhere unless a bunch of people I have no control over decides to work towards that goal.

Anyway, if you still feel like this is WTF, I'm going to close this, but I honestly think you're on the wrong track.

kallewoof force-pushed on May 16, 2020

kallewoof commented at 4:26 AM on May 16, 2020: member

Actually, I updated the dev recommendations to the following:

Note: when adding a todo entry, consider whether it would rather be better suited as an issue on github instead. TODO entries often become stale or outdated, where issues provide a higher degree of interactivity.

kallewoof renamed this:
~~doc: add note about preferring issues over TODO source code comments~~
doc: add note about considering making issues over TODO source code comments
on May 16, 2020

doc: add note about considering making issues over TODO source code comments a41eb69ee9

kallewoof force-pushed on May 16, 2020

kallewoof commented at 5:36 AM on May 16, 2020: member

Actually, I'm no longer convinced this will do anything productive. Closing.

kallewoof closed this on May 16, 2020

kallewoof deleted the branch on May 16, 2020

DrahtBot locked this on Feb 15, 2022