New TODO format?

[Peter Boström]

Hi folks,

It looks like go/todostyle (internal) has been updated to prefer the format:

// TODO: crbug.com/12345 - We should remember to do the thing.

over our existing // TODO(crbug.com/12345): Foo. format. This seems to integrate better with IDEs. I see the new format mentioned in the style guide but it seems like there's no preference for either.

I think I'd like to propose updating the style guide to list the rationale in go/todostyle and recommend the preferred format more strongly than others.

I am not at this point suggesting any mass reformat (though go/todostyle provides a handy macro we could just do this with a LSC, no IWYU pain involved).

Any objections or thoughts about this? WDYT?

Peter

[Dirk Pranke]

What does "integrate better with IDEs" mean in this case?

-- Dirk

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGFX3sEP44sc1N0hg%2BVjM9rxwqauo1s8j5jFWvOunF4Skov%2Bfg%40mail.gmail.com.

[Peter Boström]

I would guess in general that it's mostly highlighting the TODO. Doesn't look like it's built into VSCode, but there are extensions for it. I could be wrong.

Internal snippet says "External IDEs have converged on the TODO: format, and thus this is now considered a legacy form.", but I don't have the data sources for it.

[Alex Cooper]

FWIW for me VSCode doesn't auto highlight/linkify crbug.com/1234, but does if it's https://crbug.com/1234, which is a separate but related issue.

Externally I do feel that I see "TODO:" style more often than "TODO(Foo)", but oftentimes it is a bare "TODO:" where we e.g. want to enforce a bug or person (with I think strong preference being given for a bug).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGFX3sH4U5UvsoHZR67MqtF-otpkyk9gegdkHxWA_w52chUnTQ%40mail.gmail.com.

[Peter Boström]

FWIW I'd be supportive in this direction too, making our links clickable in IDEs and without hassle is a good thing and // TODO: https://crbug.com/<id>: Isn't that painful to write if a resubmit tells you to not write any of the other formats.

[danakj]

This feels like a big waste of time to change to me? What IDEs are highlighting this better in a way that is worth spending money on?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGFX3sFj6-n6%2BYRrm3OGKRFwdrYa2fMQuzi_EDYQvqx6Ehq2qg%40mail.gmail.com.

[Dirk Pranke]

Recognizing that we're bikeshedding at this point on something I would consider very low value, I'll only say that for me the "https://" would be a bunch of noise I'd prefer to avoid.

-- Dirk

[Alex Cooper]

> This feels like a big waste of time to change to me? What IDEs are highlighting this better in a way that is worth spending money on?

I don't think it's worth enforcing any change; but I and my immediate co-workers do try to use the https:// version because then we can directly open the links with an alt/ctrl+click directly from VSCode rather than having to copy them out. I'm sure there's probably some way (perhaps even the Chromium IDE does this) to autolinkify even just bare crbug.com/ links.

Regardless, I think the more important question Peter raised if we should align our TODO comment style with the upstream (e.g. TODO: Foo vs. TODO(Foo)).

[danakj]

On Fri, Feb 9, 2024 at 4:49 PM Alex Cooper <alco...@chromium.org> wrote:

> This feels like a big waste of time to change to me? What IDEs are highlighting this better in a way that is worth spending money on?

I don't think it's worth enforcing any change; but I and my immediate co-workers do try to use the https:// version because then we can directly open the links with an alt/ctrl+click directly from VSCode rather than having to copy them out. I'm sure there's probably some way (perhaps even the Chromium IDE does this) to autolinkify even just bare crbug.com/ links.

Regardless, I think the more important question Peter raised if we should align our TODO comment style with the upstream (e.g. TODO: Foo vs. TODO(Foo)).

My belief is no until there's some strong value here that I am not currently seeing.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGOLbz3fG1yse3jCEdcRE3MR%2BRH8mkgKO_3ksemc4X8kaszvTw%40mail.gmail.com.

[Alexei Svitkine]

Just a thought, but if the reason we're considering "https://" prefixes is VSCode, could we file a bug with them or contribute a patch to it to make it work with unprefixed links?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaTK7HPDJptjT%2BOTF%3DqQ7Sb_b%2BtMXRaO9QaHTpsj%2BVys0w%40mail.gmail.com.

[Dan Harrington]

I think doing this in a vscode extension is pretty easy. In fact, it looks like this is supposed to be provided by go/chromiumide, but it doesn't seem to work for me. https://g3doc.corp.google.com/chrome_platform/eng_velocity/g3doc/chromiumide/syntax_highlighting.md#linking

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAKFU2SBXiVY91Qy%3D4S%3DfmRM1J28qmDdSCXvjqmVB2_PVKaYu2w%40mail.gmail.com.

--

Dan

[Daniel Cheng]

1. I don't think we should change existing TODOs.

2. I think new TODOs should reasonably follow the updated style guidance though.

Also, I don't really like embedding https:// because 80 columns. But this is a personal preference and not one I would ask for a change for (either way) in a review :)

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAPQMVQPHLCseumBLsQjUA2b6XUDoDK60zXCXgefXkjw5BieEnA%40mail.gmail.com.

[Lei Zhang]

+1.

The tool should be able to figure out crbug.com/NNN is a link and make
it clickable, without the https:// in front.

> To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKppGDmxH%3DAtivU2OQRaZNe923-DdY_W2mKz8HRTJFPbtQ%40mail.gmail.com.

[Peter Kasting]

+1 to all of this.

The less we give explicit guidance and just let the Google style guide stand on its own, the better.

PK

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKppGDmxH%3DAtivU2OQRaZNe923-DdY_W2mKz8HRTJFPbtQ%40mail.gmail.com.

[Christoph Schwering]

The Chromium style guide recommends "TODO(crbug.com/123):" over "TODO: crbug.com/123 -" since crrev.com/c/5598198.

Is that consensus? My reading of this thread was that Chromium should follow go/todo-syntax.

Chris

[Peter Kasting]

Alison, can you give more context on that cl? There's no bug link and no real discussion of background or history, and since it wasn't discussed here or reviewed by the style guide owners, I'm not sure how to comment. 

PK

[Joe Mason]

I believe Alison is OOO but this comment on go/crbug-todo-migration requested the style guide update: https://docs.google.com/document/d/1moEwYhXgUL-QzhC7K82QlQqffhyCBtUfB9EBi8B8IE4/edit?disco=AAABPKzRKUQ

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDt2GKaz6OfrOofOedKsuvrLYYAqeQkHtx1tt%2BQqJo%2Bgg%40mail.gmail.com.

[Peter Kasting]

On Tue, Jun 25, 2024 at 9:40 AM Joe Mason <joenot...@google.com> wrote:

I believe Alison is OOO but this comment on go/crbug-todo-migration requested the style guide update: https://docs.google.com/document/d/1moEwYhXgUL-QzhC7K82QlQqffhyCBtUfB9EBi8B8IE4/edit?disco=AAABPKzRKUQ

I see. I added a comment there and on the CL. It looks from Alison's calendar like she may be back in office so I'll let her respond further rather than just reverting.

PK

[Alison Gale]

Catching up on this thread after my time away. As someone suggested above, my update to the style guide was added mostly to emphasize the crbug.com/XXX format (to avoid naked numbers or b/XXX) and I wanted to avoid explicitly pushing the new google3 internal version that hadn't been widely adopted internally since I hadn't seen any announcements about it. It looks like the change from "TODO(b/X):" to "TODO b/X - " in the Google style guide happened a year ago and so I made the style guide update just match what the current practice is. It wasn't intended to serve as a change from the new google3 style but rather to codify the old style and serve as a place where any new styles could be codified.

That being said, if we want to go with the new version, it might be useful to announce that internally because it'll probably be a while before folks find the docs and it would be nice to have consistency. I have no problem with that version (though the inconsistencies of the codebase will bother me but I'll deal with that). 

Alison

[Alison Gale]

I'm reverting my update to consolidate the TODO style in: https://chromium-review.googlesource.com/c/chromium/src/+/5659428

After that revert goes in the documentation will revert back to a state where only the Java Style guide and C++ Dos and Donts provide guidance on how to format TODOs. The old guidance in both those locations, and before my changes, are two formats:

TODO(crbug.com/X): Description

TODO(username): Description

Hopefully that will serve as a good starting point for any further discussions about whether to formally adopt the new Google internal TODO format.

Alison