On 06/25/2015 06:32 PM, Francesco Ariis wrote:
On Thu, Jun 25, 2015 at 05:55:47PM +0200, Bardur Arantsson wrote:
I noticed another thing while perusing the source code: There seem to be quite a few TODO comments scattered about.
Is there some sort of convention whereby it is permitted to add TODOs as long as the person doing so pinkie-promises to fix/remove them later? Or is it somehow related to severity? Or is it just that people couldn't be bothered to make issues and just have them in out-of-band instead? Is it all just legacy from before issue tracking was introduced? Do people spontaneously clean up TODOs that others have left behind?
I recently released a little something [1] to scan for TODOs in a codebase (after being overwhelmed by them), so after your comment I felt compelled to see how cabal and its repo would fare!
Oh, yes, I actually saw your announcement and should have remembered to use it :). Just to take a tiny sample from the top: 40 Not sure about JHC 41 This file should probably be removed. Are these useful TODOs to have? Who (if anybody) is going to do anything about them?
I attach the output: there are a lot of todos but the number more or less on the same level with projects of similar complexity. I guess it is inevitable.
Unless you just ban them. Don't accept code which has TODOs in pull requests -- it's really pretty simple According To Me(TM). Unless it's your sole way of tracking issues, there are three categories of TODOs: 1) The ones that are imporant enough to become Issues in a tracker. Put them in the tracker. 2) The ones that aren't imporant enough to become Issues in a tracker. Remove them or change them to a little explanatory comment in case someone should find that place again during debugging. (Just in case you were wrong that it was a small/unimportant issue.) In some cases I find it more useful to put them in the commit comment as explanation -- commit messages *can* be a useful way to track such things. See e.g. Linux's general policy wrt. commit messages. (Granted, GitHub makes it harder to ensure commit message quality.) 3) No, actually I think those two categories are probably enough. That's it. Simple, as long as reviewers are willing to enforce the rule. (Actually, I kind of lie. I don't think they should necessarily be completely forbidden, but they *must* have a direct reference to an *open* issue.in a tracker. This is just to make the reference from the issue tracker to the relevant code more robust during unrelated changes to the code, i.e. it just serves as a "marker" for robust linking. This should ideally be checked automatically by periodic/on-pull-req code validation.) The reasoning for getting rid of TODOs entirely is simply that they are another distraction that you don't need when maintaining code: Either it's serious enough that it has to be addressed, or it's not serious enough... in which case you shouldn't be distracted by it while maintaining surrounding/adjacent code. Regards,