RFC: Adding TODO(crbug.com/#####) to the Chromium style guide

[Stuart Morgan]

The internal C++ style guide recommends one of two formats for TODOs:

- TODO(username): ...

- TODO(<bug reference>): ...

However since the second form is specific to an internal link format, it doesn't appear in the public style guide that Chromium's style refers to.

There are advantages to the second form: it doesn't become stale as people change teams/roles, it puts the arguably more important information (which bug has the context, both initial and any subsequent discussion, rather than who knew the most at the time of writing) at the beginning of the comment rather than the end, and it emphasizes the need to consider filing a bug for a TODO (if it's not worth tracking as a bug, is it *really* a TODO, rather than just a comment about the current state of the code?)

There is a scattering of the second format in Chromium already, in the form of:

TODO(crbug.com/#####): ...

and after discussions about the state of TODOs in the iOS portions of Chromium, we (those of us primarily working on iOS) have started adopting that format. There's been some concern though that since the Chromium style guide doesn't specifically allow this format, it shouldn't be used.

Does anyone object to adding that format to the Chromium style guide, to bring it in line with the conceptual recommendations of the guide that it ultimately derives from?

-Stuart

[Dana Jansens]

I already ask people to file a bug and add it to the TODO's text when it makes sense, so that there's something to easily track and say "can we do this yet?" or similar questions and context. Having the author's name (most of the time it's the author's name that is used) in the TODO adds nothing that git blame doesn't tell you anyway.

I don't know that this creates any extra pressure to add bug links, as authors can always just put a name and not file a bug in either case. Maybe it does to avoid forever taking responsibility with your name. To me I think the strong argument is consistency with the style guide.

 

-Stuart

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Elliott Sprehn]

On Mon, Oct 26, 2015 at 2:50 PM, Dana Jansens <dan...@chromium.org> wrote:

On Mon, Oct 26, 2015 at 2:29 PM, Stuart Morgan <stuart...@chromium.org> wrote:

The internal C++ style guide recommends one of two formats for TODOs:

- TODO(username): ...

- TODO(<bug reference>): ...

However since the second form is specific to an internal link format, it doesn't appear in the public style guide that Chromium's style refers to.

There are advantages to the second form: it doesn't become stale as people change teams/roles, it puts the arguably more important information (which bug has the context, both initial and any subsequent discussion, rather than who knew the most at the time of writing) at the beginning of the comment rather than the end, and it emphasizes the need to consider filing a bug for a TODO (if it's not worth tracking as a bug, is it *really* a TODO, rather than just a comment about the current state of the code?)

There is a scattering of the second format in Chromium already, in the form of:

TODO(crbug.com/#####): ...

and after discussions about the state of TODOs in the iOS portions of Chromium, we (those of us primarily working on iOS) have started adopting that format. There's been some concern though that since the Chromium style guide doesn't specifically allow this format, it shouldn't be used.

Does anyone object to adding that format to the Chromium style guide, to bring it in line with the conceptual recommendations of the guide that it ultimately derives from?

I already ask people to file a bug and add it to the TODO's text when it makes sense, so that there's something to easily track and say "can we do this yet?" or similar questions and context. Having the author's name (most of the time it's the author's name that is used) in the TODO adds nothing that git blame doesn't tell you anyway.

Admittedly having to blame every time you want to figure out who added a TODO is pretty time consuming, I'd prefer we kept with the TODO(name) and put the bug link in the text. :)

- E

[Antoine Labour]

On Mon, Oct 26, 2015 at 2:50 PM, Dana Jansens <dan...@chromium.org> wrote:

On Mon, Oct 26, 2015 at 2:29 PM, Stuart Morgan <stuart...@chromium.org> wrote:

The internal C++ style guide recommends one of two formats for TODOs:

- TODO(username): ...

- TODO(<bug reference>): ...

However since the second form is specific to an internal link format, it doesn't appear in the public style guide that Chromium's style refers to.

There are advantages to the second form: it doesn't become stale as people change teams/roles, it puts the arguably more important information (which bug has the context, both initial and any subsequent discussion, rather than who knew the most at the time of writing) at the beginning of the comment rather than the end, and it emphasizes the need to consider filing a bug for a TODO (if it's not worth tracking as a bug, is it *really* a TODO, rather than just a comment about the current state of the code?)

There is a scattering of the second format in Chromium already, in the form of:

TODO(crbug.com/#####): ...

and after discussions about the state of TODOs in the iOS portions of Chromium, we (those of us primarily working on iOS) have started adopting that format. There's been some concern though that since the Chromium style guide doesn't specifically allow this format, it shouldn't be used.

Does anyone object to adding that format to the Chromium style guide, to bring it in line with the conceptual recommendations of the guide that it ultimately derives from?

I already ask people to file a bug and add it to the TODO's text when it makes sense, so that there's something to easily track and say "can we do this yet?" or similar questions and context. Having the author's name (most of the time it's the author's name that is used) in the TODO adds nothing that git blame doesn't tell you anyway.

There's often cases where the TODO name doesn't match git blame. E.g. I sometimes ask during reviews for someone to add a TODO(piman), because I know the details but the CL author may not have all the subtleties in mind.

Keep in mind that the name in a TODO is not about documenting who's on the hook to fix whatever is to be fixed, but about who is knowledgeable about the issue and can be contacted for details.

Antoine

 

I don't know that this creates any extra pressure to add bug links, as authors can always just put a name and not file a bug in either case. Maybe it does to avoid forever taking responsibility with your name. To me I think the strong argument is consistency with the style guide.

 

-Stuart

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

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

[Jackie Quinn]

The question is: Can we add "// TODO(crbug.com/######): ... " as an acceptable alternative in the Chromium style guide (keeping TODO(username) as also an acceptable practice)? 

The person to reference can and should already be at least CC'ed in the reference bug, and probably explicitly pointed out in the comments. One problem the iOS team ran into was that when there was no good person to list in the TODO, we would just put TODO(ios), which doesn't provide any relevant info. By preferring TODO(crbug.com/######), it is less likely that we will forget to add a bug, and we can track all the relevant context through the bug rather than by changes to comments. We are doing this in internal iOS code, and would like to also do this in our upstream code, especially as we move more upstream.

[Evan Stade]

I'm in agreement with Elliott.

On Mon, Oct 26, 2015 at 3:45 PM, Jackie Quinn <jyq...@chromium.org> wrote:

The question is: Can we add "// TODO(crbug.com/######): ... " as an acceptable alternative in the Chromium style guide (keeping TODO(username) as also an acceptable practice)? 

The person to reference can and should already be at least CC'ed in the reference bug, and probably explicitly pointed out in the comments. One problem the iOS team ran into was that when there was no good person to list in the TODO, we would just put TODO(ios), which doesn't provide any relevant info.

I agree TODO(ios) is bad. If the issue is that sometimes you don't know who to put there, I don't see how a crbug link helps. You still need to have someone CC'd or owning the bug. I actually don't understand how it can be hard to find a name to put in the TODO. Surely the person who authors the TODO or requests it to be added is always a good candidate?

 

By preferring TODO(crbug.com/######), it is less likely that we will forget to add a bug, and we can track all the relevant context through the bug rather than by changes to comments. We are doing this in internal iOS code, and would like to also do this in our upstream code, especially as we move more upstream.

"Prefer" makes it sound like TODO(username) is unpreferred and that forgetting to add a bug is a bad thing. But most TODOs don't have bug links and I think that's fine, desirable even. If TODO(buglink) is only optional, I don't see how it makes it any more or less likely to cause someone to file a bug.

 

[Dirk Pranke]

I personally prefer TODO(crbug.com/XYZ) for the reasons Stuart gave. In addition, the bug status can (at least sometimes) be used as a clue to whether

the TODO is even still relevant.

Usually one can glean the people to check with about the TODO from the bug, and this also lets you see that there might be multiple people to check

with, rather than a single person.

While I wouldn't go so far as to try and force my preference on the whole list (i.e., I wouldn't say that we should *all* prefer it), I do think it should be

an allowable option.

-- Dirk

--

[Ryan Sleevi]

Indeed, this is frequently something I request for incoming reviews. After seeing a surprising number of features and code abandoned as the effort/experiment is wound down or the original authors transition off to new teams or companies, I find it one of the few reliable ways to avoid losing the knowledge and context of the why, and not just the what.

Granted, it may just be that I'm accustomed to seeing CLs of features "not yet finished" (for which a bug is arguably more apropros) rather than "Maybe X, maybe Y; it was considered but not implemented and talk to Z@ to find out more"

[Václav Brožek]

If we put bug links to TODO, could we make them clickable in Codesearch and working outside Google network?

Good: TODO(http://crbug.com/123)

Bad: TODO(crbug.com/123), TODO(crbug/123), TODO(http://crbug/123)

I brought this up with the Chrome on iOS team, and they prefer dropping the http:// in iOS code: XCode does not make the "good" example clickable anyway, so let's save space. But in general Chromium, CodeSearch is likely more broadly used than XCode, so let's optimize for convenience in CodeSearch.

Cheers,

Vaclav

On Tue, 27 Oct 2015 at 02:27 Ryan Sleevi <rsl...@chromium.org> wrote:

Indeed, this is frequently something I request for incoming reviews. After seeing a surprising number of features and code abandoned as the effort/experiment is wound down or the original authors transition off to new teams or companies, I find it one of the few reliable ways to avoid losing the knowledge and context of the why, and not just the what.

Granted, it may just be that I'm accustomed to seeing CLs of features "not yet finished" (for which a bug is arguably more apropros) rather than "Maybe X, maybe Y; it was considered but not implemented and talk to Z@ to find out more"

--

[Stuart Morgan]

On Tue, Oct 27, 2015 at 6:25 AM Brett Wilson <bre...@google.com> wrote:

The style guide gives the TODO format

Right, and the suggestion here is that we update our style guide to allow both formats, so that it matches the internal style guide.

Usually the Chromium position is that we should match the Google style guide except when there is a compelling reason to be different. Could those opposed to this approach explain why they think this is a case where Chromium has a specific reason to deviate from the original guide?

-Stuart

[Václav Brožek]

I stand corrected, explained below.

On Tue, 27 Oct 2015 at 11:45 Václav Brožek <va...@chromium.org> wrote:

If we put bug links to TODO, could we make them clickable in Codesearch and working outside Google network?

Good: TODO(http://crbug.com/123)

Bad: TODO(crbug.com/123), TODO(crbug/123), TODO(http://crbug/123)

I brought this up with the Chrome on iOS team, and they prefer dropping the http:// in iOS code: XCode does not make the "good" example clickable anyway, so let's save space.

XCode actually does make the link clickable, but the URL includes the closing ")". Once it becomes a link in XCode, it is harder to just copy the correct portion of it, to paste into the browser. Workarounds include appending a space or # at the end of the URL, but concerns are that making these rules too complicated will result in people forgetting what the rules were.

A number of experienced people thought this through and chose a reasonable solution, so I'll just shut up. Sorry for the noise!

Vaclav

[Aaron Gable]

I'm strongly in favor of putting "TODO(crbug.com/NNNNNN)" in the style guide as an acceptable format. Honestly, I've been doing that for three years (admittedly in the Infra code) and hadn't even noticed that it wasn't in the style guide already. The vast majority of the time, the person who would have otherwise been inside the () will be the reporter or owner of the referenced bug. And this way, the person stumbling across the TODO gets to read the bug history and have context before contacting that person, so the resulting conversation can be shorter and more productive. I see zero downsides.

Aaron

[Colin Blundell]

On Tue, Oct 27, 2015 at 5:53 PM Aaron Gable <aga...@chromium.org> wrote:

I'm strongly in favor of putting "TODO(crbug.com/NNNNNN)" in the style guide as an acceptable format. Honestly, I've been doing that for three years (admittedly in the Infra code) and hadn't even noticed that it wasn't in the style guide already. The vast majority of the time, the person who would have otherwise been inside the () will be the reporter or owner of the referenced bug. And this way, the person stumbling across the TODO gets to read the bug history and have context before contacting that person, so the resulting conversation can be shorter and more productive. I see zero downsides.

Aaron

+1

[Stuart Morgan]

On Mon, Oct 26, 2015 at 5:08 PM Evan Stade <est...@chromium.org> wrote:

I don't see how a crbug link helps. You still need to have someone CC'd or owning the bug. I actually don't understand how it can be hard to find a name to put in the TODO. Surely the person who authors the TODO or requests it to be added is always a good candidate?

I don't want to dwell on this since it's just one very specific case, not the reason for the proposal, but it's very often the case when merging upstream code into the downstream repository that the merger needs to leave a TODO without knowing who the right person is. Routing and re-assigning a bug is far lighter weight that updating a TODO comment.

-Stuart

[Brett Wilson]

I didn't realize that the internal and external style guide diverged here. If the internal style guide says something, I think we should allow it.

Brett 

[Aaron Gable]

Oh, and TODO(username) gets stale. People leave the project all the time. It's much easier to make sure that bugs remain assigned/cc'd to appropriate people than it is to grep the entire codebase and update each of their TODOs to point at someone appropriate and new.

--

[Mark Mentovai]

Elliott Sprehn wrote:

Admittedly having to blame every time you want to figure out who added a TODO is pretty time consuming, I'd prefer we kept with the TODO(name) and put the bug link in the text. :)

Clicking on a bug link and seeing who filed it, owns it, or appears to be knowledgeable about it, is easier and faster than running git blame.

As many people have commented, it’s long been considered good (if unwritten) practice to file a bug for at least some TODOs and to include a reference in the text. When a bug does exist for a TODO, typically that’s where you’d want to look for more information before consulting the TODO owner or author anyway.

If we had any automated tools that parsed and dealt with TODOs, we might want to be more restrictive, but I don’t think that we do in Chrome. And even if we did, teaching them to follow bug links to resolve an owner wouldn’t be arduous either.

[Stuart Morgan]

On Mon, Oct 26, 2015 at 2:29 PM Stuart Morgan <stuart...@chromium.org> wrote:

The internal C++ style guide recommends one of two formats for TODOs:

- TODO(username): ...

- TODO(<bug reference>): ...

However since the second form is specific to an internal link format, it doesn't appear in the public style guide that Chromium's style refers to.

It looks like the public style guide will be updated to include language about referring to either a person or a bug identifier, in case that changes anyone's view on whether this should be allowed in Chromium code.

If we aren't going to actively ban it (and it doesn't seem like there's a lot of support for doing that at this point), it may still be worth adding an example of how to format the bug reference in Chromium for clarity, either TODO(crbug.com/###) as initially proposed here, or one or more of the other options instead/as well.

-Stuart

[Nasko Oskov]

I would like us to enforce https on any links we put in. I've been asking on all reviews that I do that the link is not plaintext.

[Dana Jansens]

Why? crbug.com/XYZ ends up at https://crbug.com/XYZ.

[Nasko Oskov]

Yes, but it happens through a server redirect in plaintext. A man-in-the-middle can easily redirect you anywhere else and phish credentials from you. Being explicit with https:// prefix is going to alleviate this problem, as the world isn't yet in a position where default scheme should be considered https.

[Stuart Morgan]

$ curl -I 'http://crbug.com/12345'

HTTP/1.1 302 Found

Date: Fri, 30 Oct 2015 14:08:12 GMT

Server: Apache

Location: http://code.google.com/p/chromium/issues/detail?id=12345

Vary: Accept-Encoding

Content-Type: text/html; charset=iso-8859-1

$ curl -I 'https://crbug.com/12345'

HTTP/1.1 302 Found

Date: Fri, 30 Oct 2015 14:08:47 GMT

Server: Apache

Location: http://code.google.com/p/chromium/issues/detail?id=12345

Content-Type: text/html; charset=iso-8859-1

If this is a significant concern, then the crbug.com redirect needs to be fixed to use https, or we have the same problem one step later in the process. It also seems like configuring crbug.com to use HSTS would be a better solution than trying to enforce that everyone always use the longer form of bug references everywhere (which seems unlikely to be successful).

-Stuart

[Peter Kasting]

The fact that the internal style guide allows this, and that the external guide is going to be updated to allow this, is compelling.  We should allow this.

I don't think it's necessary to officially specify a style for bug links.  We should have a reasonably high bar for what we bother to add to our style guide, and only be explicit about things where consistency really matters.  Practically speaking, it's not significantly easier to understand or follow a bug link if it's "bug 123", "crbug.com/123", "https://crbug.com/123", etc.  I don't think the readability benefit of being consistent about this is worth the cost of having everyone read a longer style guide with more things to remember -- especially if we're allowing multiple TODO formats anyway.  I also don't think the HTTPS concern is worth demanding that; it's a concern that's as relevant (or irrelevant) to every other kind of HTTP link in the world, and I disagree with making one-off rules for bug links in TODOs for issues that fundamentally aren't related to bug links in TODOs.  If we're hugely concerned about this, make the bug tracker use HSTS; otherwise do nothing.

My one practical concern with bug links is that Chrome leadership has expressed a pretty strong preference recently for not having long-term open bugs in the bug tracker, i.e. not treating the tracker as a record of possible fixes and improvements, but only using it as a concrete work-scheduler for tasks with a definite assignee and schedule.  I disagree with this usage, but if it becomes the norm, then these TODO bugs will be at high risk of just being closed due to age, at which point it becomes very unclear when trying to read the code if the TODO is actually still valuable or not.  Perhaps TODO bugs could have some sort of label that prevents auto-closure due to inactivity.

PK

[Dirk Pranke]

On Fri, Oct 30, 2015 at 12:52 PM, Peter Kasting <pkas...@chromium.org> wrote:

The fact that the internal style guide allows this, and that the external guide is going to be updated to allow this, is compelling.  We should allow this.

I don't think it's necessary to officially specify a style for bug links.  We should have a reasonably high bar for what we bother to add to our style guide, and only be explicit about things where consistency really matters.  Practically speaking, it's not significantly easier to understand or follow a bug link if it's "bug 123", "crbug.com/123", "https://crbug.com/123", etc.

Practically speaking, the latter two can be copied into a browser and opened trivially; the former can't. 'https://' adds an extra 8 characters that don't really seem to buy much, but I'm not the stickler for TLS that others are :).

 

  I don't think the readability benefit of being consistent about this is worth the cost of having everyone read a longer style guide with more things to remember -- especially if we're allowing multiple TODO formats anyway.  I also don't think the HTTPS concern is worth demanding that; it's a concern that's as relevant (or irrelevant) to every other kind of HTTP link in the world, and I disagree with making one-off rules for bug links in TODOs for issues that fundamentally aren't related to bug links in TODOs.  If we're hugely concerned about this, make the bug tracker use HSTS; otherwise do nothing.

My one practical concern with bug links is that Chrome leadership has expressed a pretty strong preference recently for not having long-term open bugs in the bug tracker, i.e. not treating the tracker as a record of possible fixes and improvements, but only using it as a concrete work-scheduler for tasks with a definite assignee and schedule.

Oh? I've never heard this, unless one was to infer it from the discussions of bug bankruptcy, which I wouldn't necessary do.

-- Dirk

 

  I disagree with this usage, but if it becomes the norm, then these TODO bugs will be at high risk of just being closed due to age, at which point it becomes very unclear when trying to read the code if the TODO is actually still valuable or not.  Perhaps TODO bugs could have some sort of label that prevents auto-closure due to inactivity.

PK

--

[Mark Mentovai]

Stuart Morgan wrote:

$ curl -I 'http://crbug.com/12345'

HTTP/1.1 302 Found

Date: Fri, 30 Oct 2015 14:08:12 GMT

Server: Apache

Location: http://code.google.com/p/chromium/issues/detail?id=12345

Vary: Accept-Encoding

Content-Type: text/html; charset=iso-8859-1

$ curl -I 'https://crbug.com/12345'

HTTP/1.1 302 Found

Date: Fri, 30 Oct 2015 14:08:47 GMT

Server: Apache

Location: http://code.google.com/p/chromium/issues/detail?id=12345

Content-Type: text/html; charset=iso-8859-1

If this is a significant concern

I think it is for all of our infrastructure.

then the crbug.com redirect needs to be fixed to use https, or we have the same problem one step later in the process.

I filed https://crbug.com/549220 for this yesterday.

 

It also seems like configuring crbug.com to use HSTS would be a better solution than trying to enforce that everyone always use the longer form of bug references everywhere (which seems unlikely to be successful).

As far as comments in code go, we can enforce the proper https form with a PRESUBMIT check.

I do think that this is worthwhile. It’s important for our team to be aware that when https is an option, it should always be chosen.

[Peter Kasting]

On Fri, Oct 30, 2015 at 1:03 PM, Dirk Pranke <dpr...@chromium.org> wrote:

On Fri, Oct 30, 2015 at 12:52 PM, Peter Kasting <pkas...@chromium.org> wrote:

The fact that the internal style guide allows this, and that the external guide is going to be updated to allow this, is compelling.  We should allow this.

I don't think it's necessary to officially specify a style for bug links.  We should have a reasonably high bar for what we bother to add to our style guide, and only be explicit about things where consistency really matters.  Practically speaking, it's not significantly easier to understand or follow a bug link if it's "bug 123", "crbug.com/123", "https://crbug.com/123", etc.

Practically speaking, the latter two can be copied into a browser and opened trivially; the former can't.

Sure.  But how many TODO bug links do you open a day and how much time do you lose opening the bug tracker yourself?  I agree that the latter can be copied and pasted, I just don't think that benefit is worth writing into the style guide that Your TODO Links Shall Be Formatted This Way.

 

My one practical concern with bug links is that Chrome leadership has expressed a pretty strong preference recently for not having long-term open bugs in the bug tracker, i.e. not treating the tracker as a record of possible fixes and improvements, but only using it as a concrete work-scheduler for tasks with a definite assignee and schedule.

Oh? I've never heard this, unless one was to infer it from the discussions of bug bankruptcy, which I wouldn't necessary do.

Speak personally to e.g. Anthony or Tyler :)

PK