---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
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.
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.
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.
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.
--
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"
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"
--
The style guide gives the TODO format
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.
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
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?
--
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. :)
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.
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
--
$ curl -I 'http://crbug.com/12345'HTTP/1.1 302 FoundDate: Fri, 30 Oct 2015 14:08:12 GMTServer: ApacheVary: Accept-EncodingContent-Type: text/html; charset=iso-8859-1$ curl -I 'https://crbug.com/12345'HTTP/1.1 302 FoundDate: Fri, 30 Oct 2015 14:08:47 GMTServer: ApacheContent-Type: text/html; charset=iso-8859-1If 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).
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.
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.