Define coding standards for @todos

As a counter-point, having a @todo can be a good indication of where someone else could jump in with a patch to help improve the code base.

Even if we do recommend their removal from docblocks, we should still have some central listing of inline todos in the API docs. Many IDEs do that, phpDocumentor does that, and it provides a good central list of "hey, you could write a patch for this!"

Depends when you think about it, really. There's certainly value to a horribly ugly code block having a "@todo This only scales up to 50 items. We should probably convert it to something that will scale better, but I don't think we can until we get PHP 5.3" right above it. That way, someone reading through the code can see "Oh god that's horrible and it won't scale... Well, at least they know about it and want to fix it. Hey, I do know of a faster way. Let me go write a patch..."

Or even just "@todo This is a dummy implementation for the moment; we'll clean it up later." is useful during active development so you can check in code you know is not yet done but still reasonably functional. Lots of large modules have code like lying around. Parts of Drupal 6 even had "@todo, remove this in Drupal 7 when we can rely on PHP 5" floating about (mostly in reference to horribly ugly reference passing hacks needed to support PHP 4).

For a lot of things, inline documentation, close to what it's talking about, is the most valuable.

a) yes
b) yes
c) ideally yes
d) ideally yes

I'm personally most interested in c) + d), but didn't get around to work on it.

@todo statements are used everywhere, throughout Drupal core, and throughout most contributed modules.

We have a couple of various "todo:", "TODO:", "@TODO:" styles instead of @todo statements in Drupal core, but those will be eliminated/updated over time. After all, it's their single purpose.

Actually, I'm not quite sure what's actionable in here, so marking as needs more info.

.

Well, it's just facing reality. We don't always write "perfect" code, and there are valid cases in which we can say: Committing this code, including the 30 @todos in that patch, is far more worth than NOT committing it.

Recent real-world examples are:

- Field API (CCK/Fields in core)
- Database API revamp (DBTNG)
- File API revamp (stream wrappers)
- and also some of the new D7UX features

Fact is: All of those introduced new @todos, and for good reason. Contributors and maintainers were and still are able to understand whether there are open @todos left, and decisions can be based on that. It is much better to know about a missing or wonky piece of code than to not know anything. Which also helps in the peer-reviewing process, because it clarifies that a patch author already considered a possibility, but the code can be improved in a follow-up patch.

Lastly, most of the @todos are indeed tackled and tried to be resolved afterwards. So there is not too much to worry about here.

We don't write perfect code. We always improve status quo.

Why shouldn't they be in the doc header?

@todo This is a poor implementation, but it works for now. Make it faster later.

@todo Remove this method after we've converted everything over to the new approach.

(I think I used the latter in DBTNG on several methods, actually.)

Given that DBTNG was over half code comments before it even went in, an issue you were not involved in, I challenge your assertion that no one cares about code comments but you.

Drupal 6 shipped with @todo comments in it, such as "this is ugly because we have to do it this way in PHP 4. Fix it in PHP 5." We survived, and no bits were harmed in the making of all of those Drupal 6 web sites.

Things like that give the impression that Drupal is held together by rubber bands and is about to fall apart

Well then we'd at least have accurate marketing. :-) We're an open source project. If we have pieces held together by rubber bands, we should own up to that and document where the rubber bands are. If @todo is the most appropriate way to do that, so be it. It's also a very clear place for someone to jump in and do the to.

Really, I think you're making too big of a deal out of this. @todos are not harmful. And they're not somehow horribly embarrassing in a docblock but not embarrassing inline. All of software development is one long list of to-dos. If we didn't have them, we would stop writing code. :-)

grep -RiP "(?<!@)todo(\s?:)?" ./*

"Mixed." The current rules for @todo are derived from real world examples and real world problems.

All valid:

/**
 * ...
 * @todo Breaks foo bar.
 */

/**
 * ...
 * @todo Breaks foo bar. Consider blargh blah bar foo and turn this into a
 *   multiple purpose callback.
 */

  // @todo This breaks badly.
  do_foo();

  // @todo This breaks badly. Consider blargh blah bar foo and turn this into a
  //   multiple purpose callback.
  do_foo();

The reason for indenting any subsequent lines still belonging to the @todo is that there are cases in which a @todo may be followed by another comment, not belonging to the @todo:

  // @todo This breaks badly. Consider blargh blah bar foo and turn this into a
  // multiple purpose callback. Also, consider to make this better than before.
  // After processing, we can invoke the callback.
  do_foo();

(the same can happen in a function docblock)

And of course, the indentation is consistent with our other coding standards around Doxygen directives.

Hm. You're right that it's probably not a good idea to use @todo within another directive. But then again, how can we put @todo notes inline?

Technically, I think that this is one of the situations where we can diverge from Doxygen -- primarily because we have and use our own parser. After all, I don't see why we should limit ourselves to something, just because some external tool, which we do not use, is limited.

However, in the same way we can use @see where it makes most sense in the context of the surrounding docs, we should definitely not force a certain location for @todos.

I'd much rather have @todo as a paragraph-level element. I am pretty sure that's how PHPDoc uses it as well. There's no good reason for us to diverge from the industry standard.

See #21 for me saying that PHPDoc (which is not the same as Doxygen) also uses it as a paragraph-level tag as far as I am aware.

See #21 for me actively disagreeing with sun on that point. :-) Ideally I'd like to see us drop our custom bastardized hybrid document parser and use the one that the entire rest of the PHP universe uses, PHPDoc.

Sometimes "because everyone else does it that way" really is a good reason.

Note that I agree as well. :P ;)

@todo forms a new paragraph. The only question is what to do if you need to inject a "@todo-alike" thing into another directive/paragraph. Though we can defer that to a separate issue, or just ignore it entirely.

I realize that my last follow-up wasn't really clear. Especially the last paragraph should have been separated more from the rest... in short: @todo items should be allowed anywhere (well, just not within other paragraphs), similar to the usage of @see statements. Both heavily depend on the directly related context below or above them.

@sun: Ah, you mean @todo should be a paragraph-level element but don't bother with a required order within the docblock? I'm +1 on that.

I don't think it really matters either way; same for @see and @ingroup. Honestly I've usually put those above the other @directives, but I don't care strongly one way or another.

Sure, if not related to any context, then @todo items should ideally be placed at the very bottom of a docblock. Keeps them out of the "regular" documentation, and most often, a @todo relates to the function body, so good to have it as close as possible in that case.

So, what needs to be updated?

Todos

/**
 * @todo Remove this in D8.
 * @todo Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam
 *   nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed
 *   diam voluptua.
 */

To document known issues and development tasks in code, @todo statements may be used. Each @todo should form an atomic task. They should wrap at 80 chars, if required. Additionally, any following lines should be indented by 2 spaces, to clarify where the @todo starts and ends.

  // @todo Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam
  //   nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat,
  //   sed diam voluptua.
  // We explicitly delete all comments.
  comment_delete($cid);

@todo statements in inline comments follow the same rules as above, especially regarding indentation of subsequent comments.

Mostly cool with that.

Though the blank line between multiple contradicts our general rule:

 * @param $second
 *   There should be no newline between multiple directives (of the same type).

There's no reason to enforce a blank line between multiple @todos. Of course, there should be a blank line between a @todo and other statements.

Lastly, we shouldn't mention todo-lists as long as there are none. API module doesn't parse them correctly currently, and also doesn't group them into actual todo lists (which would additionally require to build back-reference mappings). Hopefully, the API module rewrite will ease this, but as of now, changes to API module are pretty much vaporable until the rewrite to use PGP or whatnot has been completed.

Where did we get with this? Does the API module now generate todo lists?

Small @todo cleanup in #1593696: Fix capitalization in and remove colons from @todo statements within core/lib. Please comment there if something there contradicts decisions reached here.