catapult-project / catapult

Deprecated Catapult GitHub. Please instead use http://crbug.com "Speed>Benchmarks" component for bugs and https://chromium.googlesource.com/catapult for downloading and editing source code..
https://chromium.googlesource.com/catapult
BSD 3-Clause "New" or "Revised" License
1.93k stars 563 forks source link

Scrub TODOs and comments referencing closed bugs. #3285

Open benshayden opened 7 years ago

benshayden commented 7 years ago

I noticed a few comments referencing closed bugs that should either be done because they're unblocked, or clarified that the links are for historical purposes, or removed because we have git blame.

That is not an exhaustive list, and I only looked in tracing/tracing. It's mostly (but not only) my code. :-)

This is low priority, but if we ever have the breathing room to scrub code health bugs, this could go somewhere on the list.

This also points to a bug hygiene issue: those bugs probably shouldn't have been closed until that code was updated. That could lead to a philosophical discussion on TODOs: should they be discouraged in favor of filing and using bugs? Etc.

@eakuefner @zeptonaut

zeptonaut commented 7 years ago

I like TODOs, but I totally agree that our current method of handling them and associating them with bugs sucks.

It seems like the problem here is, more than anything, a tooling problem. On my old team, people regularly turned on the dead code layer of Code Search in order to find code that could be deleted. Code Search also has a "stale TODO" layer that, for TODO(b/12345), identifies whether buganizer bug 12345 is closed.

Interestingly, it looks like the Catapult code search has these same layers, but it doesn't look like the indexer runs on them. It'd be really awesome to get something like that turned on, but I'm not sure how practical it would be: a simple script that finds Catapult and monorail bug links and uses the monorail API to determine if those bugs are closed would probably be sufficient. Then, it can be a sort of best practice for our team to occasionally go through and scrub the codebase of obsolete bugs.