Coding style proposal: FIXME -> TODO(name)

[TAMURA, Kent]

I propose a coding style change so that we use TODO(name) instead of FIXME.

<http://www.chromium.org/blink/coding-style#TOC-Comments>

Pros:

 - Consistency with Chromium coding style.

    <http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments>

Cons:

 if we'd like to fix existing FIXMEs,

 - Need to update many lines of comments 

 - Need to decide what existing FIXMEs should be converted to.  TODO(), TODO(anyone), TODO(*), or something else?

--

TAMURA Kent
Software Engineer, Google

[Daniel Cheng]

I don't feel strongly about FIXME or TODO(n) either way, but if we switch, I think it'd probably be better to just leave the legacy FIXMEs rather than trying to convert them.

Daniel

[Mike West]

On Thu, Mar 12, 2015 at 7:23 AM, Daniel Cheng <dch...@chromium.org> wrote:

On Thu, Mar 12, 2015, 11:47 TAMURA, Kent <tk...@chromium.org> wrote:

I propose a coding style change so that we use TODO(name) instead of FIXME.

<http://www.chromium.org/blink/coding-style#TOC-Comments>

This sounds very familiar: https://groups.google.com/a/chromium.org/d/msg/blink-dev/SZUeZj4CuI0/xfJuMEXhQIoJ :)

I'm still totally in favor of doing this going forward (especially given the impending repository merge), but I agree with dcheng@ that there's little to no value in updating the existing comments.

-mike

[Peter Kasting]

Strong +1 to using TODO(name) going forward since it's explicitly mandated across Chromium and Google style.

+1 also to not bothering to converting existing strings.

The previous thread had Eric commenting that FIXME is "the string to search for", but after many years of working on this project I can't say I've ever found myself just randomly grepping through the codebase for either FIXME or TOSO to see what it uncovers.  I don't think that's a use case worth maintaining a style rule divergence for.

PK

P.S. For anyone not familiar with Chromium's TODO(name) convention, the name after the TODO does NOT indicate that's the person responsible for implementing the TODO -- only that that's a good person to ask about what the TODO means.

[Dominic Cooney]

I support adopting Chromium-style TODO(name) and not changing legacy FIXME to something other updating them to TODO(name)--a real name--on an ad-hoc basis.

In addition to consistency with Chromium, I think TODO(name) encourages more effort to write thoughtful comments and--gasp--sometimes revisit them.

Dominic

[Philip Jägenstedt]

I think we should allow TODO(name), but must we ban all new uses of FIXME? In the ongoing IDL spec sync, I'm adding FIXMEs wherever something diverges from the spec. Even though "A TODO is not a commitment that the person referenced will fix the problem," I think it would look rather odd if my name is sprinkled all over our IDL files. It's not needed to find out how to fix the problem, since there's a link to the spec IDL in the same file.

If we don't want new FIXMEs, would "Note:" be an acceptable replacement in this case?

Philip

[Peter Kasting]

On Thu, Mar 12, 2015 at 9:56 AM, Philip Jägenstedt <phi...@opera.com> wrote:

I think we should allow TODO(name), but must we ban all new uses of FIXME? In the ongoing IDL spec sync, I'm adding FIXMEs wherever something diverges from the spec. Even though "A TODO is not a commitment that the person referenced will fix the problem," I think it would look rather odd if my name is sprinkled all over our IDL files. It's not needed to find out how to fix the problem, since there's a link to the spec IDL in the same file.

If we don't want new FIXMEs, would "Note:" be an acceptable replacement in this case?

I think a TODO with your name is entirely appropriate.  I'm not sure why you think it would look odd.  Indeed, since you've notated the divergences, presumably someone looking for details to help fix them would be best served by consulting you.  That's easier than having the person try to dig through blame when they can't find the answers they need.

This may be a case of what one is used to.  I spend much of my time coding in Chromium, and seeing TODO(name) is not in any way surprising.

Chromium does accept bare "TODO:"s, but they're not preferred.

PK 

[Steve Kobes]

Is TODO(bug#) acceptable?  This has advantages over TODO(name) - the bug provides additional context, is easily reassigned, and makes it clear if the TODO is obsolete.

[Nico Weber]

On Thu, Mar 12, 2015 at 2:14 PM, Steve Kobes <sko...@chromium.org> wrote:

Is TODO(bug#) acceptable?  This has advantages over TODO(name) - the bug provides additional context, is easily reassigned, and makes it clear if the TODO is obsolete.

(the usual format for this is `// TODO(name): Short summary http://crbug.com/12345`. This has the advantage that the bug link gets autolinkified in cs. See https://code.google.com/p/chromium/codesearch#search/&q=todo.*crbug&sq=package:chromium&type=cs for many examples.)

[Peter Kasting]

On Thu, Mar 12, 2015 at 11:14 AM, Steve Kobes <sko...@chromium.org> wrote:

Is TODO(bug#) acceptable?  This has advantages over TODO(name) - the bug provides additional context, is easily reassigned, and makes it clear if the TODO is obsolete.

This is likely controversial, but I normally encourage people not to link to bugs, because bugs often gain a variety of comments over time and it can be difficult to tell what the specific issue in the code is supposed to be, or, worse, if the bug is closed, whether the TODO is supposed to be removed or the bug was closed erroneously, or what.

In most cases, simply providing more detail in the comment is the easiest way for future maintainers to read about the issue directly.

It can be OK to link to bugs when the bug is really clear and doesn't accrue comments or change state, and the code comment is also really clear.  This seems to not happen a disturbing amount of time, especially since when people link to bugs they often seem to consider it a substitute for writing anything meaningful, so you get something like "TODO(pkasting): http://crbug.com/12345 Make this not suck"

(Incidentally, just posting bug numbers and not whole hyperlinks is bad anyway if we change bug tracking systems -- as we've done once already in the codebase's life!)

It's not like all this is rigidly set in stone.  Do what you can to make the comment as directly meaningful to as many people for as long as possible, and you're probably fine.

PK

[Philip Jägenstedt]

A pretty typical comment that I have added recently is "FIXME: documentURI should not be nullable" right above the documentURI attribute in Document.idl. Not far above is a link to https://dom.spec.whatwg.org/#interface-document which confirms this. Would including the name of the person who annotated the differences really be useful here? In what cases are bare TODOs acceptable in Chromium?

On a separate note, I'm a sucker for consistency, and leaving all existing FIXMEs alone would make the Blink code increasingly inconsistent locally. For a long time new contributors would have to learn which form is preferred. I would like to mass-convert them to TODO, or even better use a script to include the user name of at least FIXMEs added since Blink forked from WebKit.

Philip

[Nico Weber]

(In a few places we have "anonymous attributed TODOs". During cross-platform bringup, we had `TODO(port)`, a few gn files have `TODO(gyp)`, etc. So `TODO(blink)` could be a replacement. It probably doesn't matter much either way though.)

[Peter Kasting]

On Fri, Mar 13, 2015 at 10:36 AM, Philip Jägenstedt <phi...@opera.com> wrote:

A pretty typical comment that I have added recently is "FIXME: documentURI should not be nullable" right above the documentURI attribute in Document.idl. Not far above is a link to https://dom.spec.whatwg.org/#interface-document which confirms this. Would including the name of the person who annotated the differences really be useful here? In what cases are bare TODOs acceptable in Chromium?

Why is including your name harmful?

What happens if later someone adds 100 lines of code between these two places?  Or removes the spec link?  Or the code gets refactored and moved to a different place?  Just because there's nearby context now doesn't mean there always will be.

So yes, I think your name should be here.

On a separate note, I'm a sucker for consistency, and leaving all existing FIXMEs alone would make the Blink code increasingly inconsistent locally. For a long time new contributors would have to learn which form is preferred. I would like to mass-convert them to TODO, or even better use a script to include the user name of at least FIXMEs added since Blink forked from WebKit.

There are many places where our code is inconsistent, some of them due to style changes over time.  The authority is the style guide: today the Blink style guide explicitly says to use FIXME.  If that is changed, I don't think existing differing practice in the codebase is a problem, especially on this issue, where I just don't see a lot of new contributors wanting to write such comments, seeing lots of FIXMEs, and commenting them.

PK

[Philip Jägenstedt]

On Sat, Mar 14, 2015 at 3:38 AM, Peter Kasting <pkas...@google.com> wrote:

On Fri, Mar 13, 2015 at 10:36 AM, Philip Jägenstedt <phi...@opera.com> wrote:

A pretty typical comment that I have added recently is "FIXME: documentURI should not be nullable" right above the documentURI attribute in Document.idl. Not far above is a link to https://dom.spec.whatwg.org/#interface-document which confirms this. Would including the name of the person who annotated the differences really be useful here? In what cases are bare TODOs acceptable in Chromium?

Why is including your name harmful?

What happens if later someone adds 100 lines of code between these two places?  Or removes the spec link?  Or the code gets refactored and moved to a different place?  Just because there's nearby context now doesn't mean there always will be.

So yes, I think your name should be here.

For IDL files, the spec is the authority on what they should say. The spec's URL and content can change to make any part of our IDL files wrong, but then you need to look at the spec's revision history to understand why. I will grant that this isn't always trivial and that if there's a TODO(philipj) nearby I could be asked to help figure out what has happened.

After going through only 10% of our IDL files I've added 142 FIXMEs, about 1 every 16 lines. It's the eventual number of TODO(philipj) that makes me a bit uncomfortable, but maybe I could get used to it. Or maybe FIXME isn't the best way to annotate differences with the spec.

I'm sympathetic to an argument for consistency, however.

On a separate note, I'm a sucker for consistency, and leaving all existing FIXMEs alone would make the Blink code increasingly inconsistent locally. For a long time new contributors would have to learn which form is preferred. I would like to mass-convert them to TODO, or even better use a script to include the user name of at least FIXMEs added since Blink forked from WebKit.

There are many places where our code is inconsistent, some of them due to style changes over time.  The authority is the style guide: today the Blink style guide explicitly says to use FIXME.  If that is changed, I don't think existing differing practice in the codebase is a problem, especially on this issue, where I just don't see a lot of new contributors wanting to write such comments, seeing lots of FIXMEs, and commenting them

Why wouldn't a new contributor eventually want to write a FIXME and copy local style? Even if a presubmit hook can inform them of the problem, that same presubmit hook will trigger on refactoring for years to come. This kind of organic transition is IMHO much inferior to just writing a script and making everything consistent at the time you change the style guide. Some style changes are too tricky to do that, but this one isn't.

Philip

[TAMURA, Kent]

On Sun, Mar 15, 2015 at 8:32 PM, Philip Jägenstedt <phi...@opera.com> wrote:

On Sat, Mar 14, 2015 at 3:38 AM, Peter Kasting <pkas...@google.com> wrote:

On Fri, Mar 13, 2015 at 10:36 AM, Philip Jägenstedt <phi...@opera.com> wrote:

A pretty typical comment that I have added recently is "FIXME: documentURI should not be nullable" right above the documentURI attribute in Document.idl. Not far above is a link to https://dom.spec.whatwg.org/#interface-document which confirms this. Would including the name of the person who annotated the differences really be useful here? In what cases are bare TODOs acceptable in Chromium?

Why is including your name harmful?

What happens if later someone adds 100 lines of code between these two places?  Or removes the spec link?  Or the code gets refactored and moved to a different place?  Just because there's nearby context now doesn't mean there always will be.

So yes, I think your name should be here.

For IDL files, the spec is the authority on what they should say. The spec's URL and content can change to make any part of our IDL files wrong, but then you need to look at the spec's revision history to understand why. I will grant that this isn't always trivial and that if there's a TODO(philipj) nearby I could be asked to help figure out what has happened.

After going through only 10% of our IDL files I've added 142 FIXMEs, about 1 every 16 lines. It's the eventual number of TODO(philipj) that makes me a bit uncomfortable, but maybe I could get used to it. Or maybe FIXME isn't the best way to annotate differences with the spec.

IMO TODO(philipj) is appropriate in this case, and you don't need to be uncomfortable at all. TODO(philipj) doesn't mean you were wrong or you need to fix it.  If the comment has enough information, none will ask you about the comment.

The name part of TODO(philipj) may be helpless in this case, but is good for consistency.

 

I'm sympathetic to an argument for consistency, however.

On a separate note, I'm a sucker for consistency, and leaving all existing FIXMEs alone would make the Blink code increasingly inconsistent locally. For a long time new contributors would have to learn which form is preferred. I would like to mass-convert them to TODO, or even better use a script to include the user name of at least FIXMEs added since Blink forked from WebKit.

There are many places where our code is inconsistent, some of them due to style changes over time.  The authority is the style guide: today the Blink style guide explicitly says to use FIXME.  If that is changed, I don't think existing differing practice in the codebase is a problem, especially on this issue, where I just don't see a lot of new contributors wanting to write such comments, seeing lots of FIXMEs, and commenting them

Why wouldn't a new contributor eventually want to write a FIXME and copy local style? Even if a presubmit hook can inform them of the problem, that same presubmit hook will trigger on refactoring for years to come. This kind of organic transition is IMHO much inferior to just writing a script and making everything consistent at the time you change the style guide. Some style changes are too tricky to do that, but this one isn't.

Philip

[Philip Jägenstedt]

On Mon, Mar 16, 2015 at 7:55 AM, TAMURA, Kent <tk...@chromium.org> wrote:

On Sun, Mar 15, 2015 at 8:32 PM, Philip Jägenstedt <phi...@opera.com> wrote:

On Sat, Mar 14, 2015 at 3:38 AM, Peter Kasting <pkas...@google.com> wrote:

On Fri, Mar 13, 2015 at 10:36 AM, Philip Jägenstedt <phi...@opera.com> wrote:

A pretty typical comment that I have added recently is "FIXME: documentURI should not be nullable" right above the documentURI attribute in Document.idl. Not far above is a link to https://dom.spec.whatwg.org/#interface-document which confirms this. Would including the name of the person who annotated the differences really be useful here? In what cases are bare TODOs acceptable in Chromium?

Why is including your name harmful?

What happens if later someone adds 100 lines of code between these two places?  Or removes the spec link?  Or the code gets refactored and moved to a different place?  Just because there's nearby context now doesn't mean there always will be.

So yes, I think your name should be here.

For IDL files, the spec is the authority on what they should say. The spec's URL and content can change to make any part of our IDL files wrong, but then you need to look at the spec's revision history to understand why. I will grant that this isn't always trivial and that if there's a TODO(philipj) nearby I could be asked to help figure out what has happened.

After going through only 10% of our IDL files I've added 142 FIXMEs, about 1 every 16 lines. It's the eventual number of TODO(philipj) that makes me a bit uncomfortable, but maybe I could get used to it. Or maybe FIXME isn't the best way to annotate differences with the spec.

IMO TODO(philipj) is appropriate in this case, and you don't need to be uncomfortable at all. TODO(philipj) doesn't mean you were wrong or you need to fix it.  If the comment has enough information, none will ask you about the comment.

The name part of TODO(philipj) may be helpless in this case, but is good for consistency.

Very well, I do like consistency, and trying to draw a line between TODOs where the name is and is not needed seems hard.

It looks like there's very strong support for switching to TODO(name) for new comments, just need to figure out what to with existing FIXMEs.

Philip 

[TAMURA, Kent]

Let me conclude that:

* Use TODO(name) in Blink from now on.

* Don't replace existing FIXMEs mechanically for now.

The latter needs more discussion.

Understanding existing FIXMEs and replacing them with TODO(your-name) is welcome :)

[Philip Jägenstedt]

That sounds good, will someone also update the style guide?

If anyone writes a script to find and replace FIXMEs you've added with
TODO(self), please share :)

Philip

> To unsubscribe from this group and stop receiving emails from it, send an
> email to blink-dev+...@chromium.org.

[Levi Weintraub]

Looks like the style guide has been updated :)