we use "TODO(bug id or committer id): msg" as the format in other projects and that seems to be enough breadcrumb in most cases
-Jake On Thu, Sep 15, 2016 at 11:06 PM, Todd Lipcon <t...@cloudera.com> wrote: > On Thu, Sep 15, 2016 at 6:50 PM, Adar Dembo <a...@cloudera.com> wrote: > > > In https://gerrit.cloudera.org/#/c/4435/, Tidy Bot said: > > > > Line 209: // TODO: Should be fixed with Exactly Once semantics, > > see KUDU-1537. > > warning: missing username/bug in TODO [google-readability-todo] > > // TODO: Should be fixed with Exactly Once semantics, see > KUDU-1537. > > ^ > > // TODO(unknown): Should be fixed with Exactly Once semantics, > > see KUDU-1537. > > > > This doesn't look like the kind of style change we want, right? > > Historically we don't annotate our TODOs with names. > > > > Or should I reformat it as "TODO(KUDU-1537)..." ? > > > > Yea, I think TODO(bug#) is a good policy to try to have moving forward, but > I don't think we have to be religious about it. I can turn off this tidy > check if we think it's not worth it with a codebase of our size. > > This guideline comes from Google ( > https://google.github.io/styleguide/cppguide.html#TODO_Comments) where > they > say: > > TODOs should include the string TODO in all caps, followed by the name, > e-mail address, bug ID, or other identifier of the person or issue with the > best context about the problem referenced by the TODO. The main purpose is > to have a consistent TODO that can be searched to find out how to get more > details upon request. A TODO is not a commitment that the person referenced > will fix the problem. Thus when you create a TODO with a name, it is almost > always your name that is given. > It's sort of nice to leave a breadcrumb, but it's also not too hard for > someone to 'git blame' and figure out who added it, so I could go either > way. > > -Todd > > > > > > > > On Wed, Sep 14, 2016 at 10:04 PM, Todd Lipcon <t...@cloudera.com> wrote: > > > Hey folks, > > > > > > I've set up a jenkins job and gerrit trigger to run clang-tidy-diff on > > any > > > patches that are uploaded. It should be set up now so as not to vote +1 > > or > > > -1, but just to produce comments. For an example of the type of > warnings > > it > > > generates, check out: > > > https://gerrit.cloudera.org/#/c/4409/4/src/kudu/consensus/ > > raft_consensus_state.h > > > > > > If you see any checks that you think are false positives, feel free to > > ping > > > me and I can either disable those checks entirely, or see if there's > some > > > configuration we can make to better match our own guidelines. > > > > > > Hopefully this turns out to be a useful bit of "automatic code review" > so > > > that we can focus our review efforts less on mechanical checks and more > > on > > > things requiring human judgment :) If it turns out to be more of an > > > annoyance than a help, we can always remove it or really dial back to > > only > > > the most important warnings. > > > > > > Also worth noting that these checks are not that complicated to write, > so > > > if we see that there are some Kudu-specific ones worth implementing, we > > can > > > easily do so. > > > > > > -Todd > > > -- > > > Todd Lipcon > > > Software Engineer, Cloudera > > > > > > -- > Todd Lipcon > Software Engineer, Cloudera >
