Roman Prokofyev

Chief Scientist at FAIRTIQ. I work on making public transport accessible for everyone.

Git hooks in a battle for code quality

16 Feb 2012 » en, githooks, coding, git

Almost every VCS has some sort of a hook system that developer can leverage. At work we use Git and write on Python, and for such combination there is a great githooks app that allows to check commited files against pep8, pyflakes, pdb and jslint.

But honestly, I consider that rejecting git push just because of pep8 or pyflakes violations to be evil for projects that did not follow these rules from the very beginning. For example, ScienceWISE project is one of those, and we’re not alone. The good thing is that now we constantly keep track of all violations inside Jenkins and we try to descrease their number with every commit.

But githooks is really useful when it comes to some worst practices checking, especially if these worst practices can be expressed as a string or regex match. So I decided to fork githooks and add my own custom checker. Here are the rules I have now:

str_list = [('javascript:', 'Obtrusive javascript is forbidden'),
            ('HttpResponseNotFound', 'HttpResponseNotFound is forbidden, raise Http404 instead'),
            ('tasks.register\(', 'No need to register tasks anymore')]
re_list = [(re.compile(r'<.+onclick=.+>', re.I), 'Obtrusive javascript is forbidden (onclick)'),
           (re.compile(r'<.+style=.+>', re.I), 'Inline styles are forbidden'),
           (re.compile(r'\$\(.+#[^\s]+\)\.each'), 'Iteration on singleton elements is prohibited'),
           (re.compile(r'signals\..+?\.connect'), 'Old-style signals connect are forbidden, use @receiver'),
           (re.compile(r'class .+:\n(?!\s\s\s\s""")'), 'Not all classes have docstrings'),
           (re.compile(r'class .+?\((?:Periodic)?Task\):'), 'Old-style celery task classes are forbidden'),
           (re.compile(r'\.values_list\([^,]+(?!,flat=True)\)'), 'Didn\'t you forget to flatten quesryset?'),
           (re.compile(r'if\s?request\.method\s?==\s?'), 'Conditioning on request.method is forbidden, use decorators instead')]

As our team grows and people come and go, these simple heuristics should protect from doing really bad things, and I will try to extend and improve these lists while doing more and more code reviews :)

In future I would like to make these expressions in form of a django app, so anyone could add his custom rules.