search

plone / plone.recipe.codeanalysis

provides static code analysis for Buildout-based Python projects, including flake8, JSHint, CSS Lint, and other code checks
https://pypi.org/project/plone.recipe.codeanalysis/
11 stars 8 forks source link

Is the use of FIXME, TODO and XXX a bad practice? #193

Open idgserpro opened 8 years ago

idgserpro commented 8 years ago

When I use the recommended plone.recipe.codeanalysis, I get a T000 Todo note found and codeanalysis fail.

Is the use of FIXME, TODO and XXX a bad practice?

do3cc commented 8 years ago

Yeah, I disagree with this too, since it leads to not making these comments, even though they are true

gforcada commented 8 years ago

No, it's not a bad practice, but if you made the effort of adding them to remind you that something needs to be fixed, or that it has to be improved, or that is a gross hack, well, thanks to p.r.codeanalysis you have a reminder, or do you schedule 1 hour every week to review all your code and look if there's any TODO, FIXME, XXX around that needs some attention?

I use TODO/FIXME/XXX as a reminder that something needs to be worked on at some point, that p.r.codeanalysis reports about them is great. From time to time I get around to fix them if they indeed need some work, or after thinking about it again I decide to remove it because it's already ok what has been done...

The main question here is: why did you added them in the first place? To write and ignore them forever or to get back at it at some point later on and thus being used as a reminder?

hvelarde commented 8 years ago

the way it currently works forces you to just ignore those errors, which is equivalent to not use it at all. IMO, the right way is just to warn about the existence of those messages and don't fail on code analysis.

datakurre commented 8 years ago

:+1: for Hector's proposition. TODOs should be printed, but they should not affect the exit code.

Héctor Velarde notifications@github.com kirjoitti 20.5.2016 kello 0.31:

the way it currently works forces you to just ignore those errors, which is equivalent to not use it at all. IMO, the right way is just to warn about the existence of those messages and don't fail on code analysis.

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub

gforcada commented 8 years ago

Fair point, now, how should we do that, anyone wants to tackle the issue upstream at flake8 or do we solve it here at plone.recipe.codeanalysis? Anyone takes the task? :-)

idgserpro commented 8 years ago

I don't think it's a bad practice. I agree with @hvelarde:

the right way is just to warn about the existence of those messages and don't fail on code analysis.