How we setup pylint on a git pre-receive hook

We have worked as a cohesive team of Python developers for a long time. Either it is underscore variable names, spacing conventions or the 80-column rule, most of them had no problem. All the code written could be pretty much guaranteed to be PEP-8 compliant.

That is when, as the team grew and added members fairly new at Python (if not new at writing code professionally as well), what we thought was common knowledge flew out the window.

We tried code review and feedback on commits but that wasn't enough to catch the range of problems, keep cohesion nor help new developers step their game up. Time to enforce conventions rigorously. That is to say: mechanically.

Introducing Pylint

Since it is one of the best know tools in the Python community, we settled on pylint.

The first hurdle, and the main reason that plagues static analysis tool, is they try to do too much. False positives kill the usefulness of those tools. Turning off warnings is usually meant with a valid rationale like "Why use the tool in the first place then?".

The problem we faced was severe. We thus decided that it does not matter if we globally turn off ALL of pylint reports expect for the 80-column rule. By itself, that report was already useful to improve team cohesion.

So we hacked out. Here the standard .pylintrc developers must use:

# We develop Django projects, so we had an helpful plug-in here:
$ pip install pylint-django

$ pylint --load-plugins pylint_django --generate-rcfile > ~/.pylintrc
$ diff -u prev ~/.pylintrc
-load-plugins=
+load-plugins=pylint_django

-#disable=
+# XXX added attribute-defined-outside-init because that's the only way
+# I found to disable a warning from within django:
+#     W:148, 8: Attribute 'object' defined outside __init__
+disable=missing-docstring,too-few-public-methods,fixme,too-many-public-methods,
too-many-ancestors,attribute-defined-outside-init,too-many-branches

-reports=yes
+reports=no

Pre-receive hook

At this point, pylint already started to show usefulness, catching spelling errors, duplicate and unused imports, etc.

To ruthlessly enforce conventions, we must reject any commit that does not pass 100%. This was done as a pre-receive hook in the master git repository on the server.

That is where things got messy. The approach works well in theory, except pylint relies on ALL prerequisites of the project to be installed correctly to do its analysis. If we do not run pylint server-side in a virtualenv fully populated with the prerequisites for the project attached to the pre-receive hook, "bogus" issues are reported, and commits are rejected.

We could solve this by running pip install -r requirements.txt before running pylint but that would defy the purpose of a quick accept or reject.

Instead we decided to use the virtualenv already setup for running unit tests to run pylint into and deal with the occasional hand holding when a new prerequisite is added to the project.

That is where our security conscious mind played a trick on us. The repos are own by a git git user while the virtualenv to run unit tests is own by a jenkins user. Running ./setup.py install as jenkins would result with files installed as such:

-rw-r-----  1 jenkins jenkins __init__.py

Every time the buildbot would install a new version of a prerequisite, the next git push would be rejected because pylint (pre-receive), would run under the git user. We solved the issue by adding both user to a common group and that was it.

Conclusion

We did only embark on the pylint path as a way to help new developers learn about conventions. That switched our mentality on pylint to:

"If we can only enforce space conventions and the 80-column rule,
pylint is good enough for us."

We then were not afraid to cut, slash, and axe anything out of pylint if that meant we could just enforce those two reports.

Getting pylint to reliably run on a pre-receive hook was littered with little hurdles here and there. Getting our projects pylint clean required a fair amount of slashing reports globally (ex: too-many-public-methods) and disabling some locally (ex: unused-argument). In the end, whatever we left as pylint reports still significantly help everyone on the team to catch stupid mistakes and real errors early. We wouldn't live without the pre-receive script anymore.

photo credit: Denis Dore Photography via photopin cc