Wellfire Interactive // Django consultants for your existing Django site

Starting to prioritize and triage issues in cleaning up Django apps (This Old Pony #51)

A couple of weeks ago I wrote about using a combination of tools and features of the Python language to help assess Django apps and write them a bit more defensively.

One suggestion was to use a tool like flake8 to report not only on formatting issues but more importantly potential bugs identified through static analysis (including the kind of bugs that are discovered by a compiler _or _in an interpreted language, at runtime).

Reader Karl Goetz wrote back (shared with permission):
 

I just ran flake8 on a project and was given a list of 1845 issues. Depressing!

We discussed briefly and validated my hunch that most of these were style issues. That’s not to say they should be ignored, but that a list of over 1000 issues or more isn’t necessarily something to lose sleep over. It’s all about one, understanding which problems are important and two, having a strategy in hand to deal with these issues.

How you prioritize issues in an existing/legacy software project is one of the most significant stumbling blocks for developers and product teams working in Django apps (and pretty much any other type of project), so we have plenty of grist.

Linter reporting is not where I’d normally start, but rather than switch gears let’s start here anyhow.
 

Slicing flake8 issues

For our purposes we’re going to assume flake8 as the tool of choice, although that’s not strictly required. flake8 includes linting, complexity analysis, and static analysis, making it an excellent default choice.

Every individual error will be reported back to you with an error code telling you exactly what it is (e.g. E225 or F403, missing whitespace around an operator and starred imports, respectively). These errors have different discovery sources (e.g. pycodestyle or pyflakes) and different implications. Missing whitespace around an operator has no impact on how the code runs. It does however make the code more difficult to read and thus is likely to slow down development or even lead to programmer errors due to misreading. Starred imports similarly have no direct impact on how the code runs (e.g. from module import *) however they obscure the available namespace in a given module. This can result in shadowed imported names in a module’s namespace, and it also makes it more challenging for other tools to identify names in use. Both issues represent friction and increased potential for developer error, but the starred import could be hiding a code issue.

On the other hand, F823 - local variable referenced before assignment - represents a runtime error waiting to happen. 

I find it helpful to think of the different issues you might run into, specifically reported back by linting and static analysis, into three categories:

  1. Definite bug
  2. Could be hiding bugs
  3. Could lead developers to create bugs (and definite friction)

The definite bugs

The most obvious and uncontroversial “definite” bugs are those that will result in runtime errors. This includes things like return or yield statements outside of functions, a break statement outside of a while or for loop, and a naked except block as not the last exception handler. These should result in errors when a module is imported. Other errors, like undefined names, will never be raised until that one line of code is run. Extensive test coverage will catch errors like this, but we don’t need to run the test suite to find these issues.

In the event an undefined name needs to be imported then the solution is as simple as adding the necessary import. Often the name is presumed to have been defined in the code, as an empty list, for example. The solution here is to add in the initial definition, provided you can identify the type and expected initial value.

Other “definite” bugs are those that don’t cause runtime errors but might be expected to create bugs involving different values. For example, F601 - dictionary key name repeated with different values - won’t cause your program to crash, but is likely to result in unexpected values being assigned. 
 

Hiding bugs

While definite bugs are a higher priority, this category tends to be much larger and require more extensive fixes. These are issues that are not going to directly cause errors in your app but pose significant risk by hiding potential bugs.

We already mentioned starred imports, but the most common that comes to mind is plain, “naked” exceptions. flake8 will report these as E722, do not use bare except, specify exception instead. Exceptions get used for one of two purposes - expected control flow and rescue from errors that unnecessarily crash the program. By control flow I mean using exceptions idiomatically where in other languages (such as Go) you’d use explicit checks on values. Instead of checking for list length and returning the 5th element from a list if it’s length is at least 5 and another if it’s not, the idiomatic solution is to try returning the fifth indexed element and in the event of an IndexError return the alternate value. Whereas you might have a function that sits at the top of a call stack including various backing services (database) and HTTP APIs and the goal is to ensure that no matter what happens no error in this call stack is ever propagated back up to the user in the form of a 500 error. We may want to catch at least a base exception here, but nonetheless there is a rationale for catching everything. 

Now the _reason _for being specific, especially in the first instance, is two-fold. The explicitness of having the named exception shows intent so that other developers understand why there is a try block and also how the flow control works. In both cases, a naked exception just swallows everything so, for example, if there’s an actual bug throwing an error then it’s possible that even a test won’t catch this because all exceptions were implicitly handled. In a case like the latter where there may be too many possible exceptions to handle, from socket issues to third party API responses, the solution is to first add an exception log before continuing so that you have access to the full error information, and to separately handle known/expected exceptions first. That might look something like this:

try: do\_something() except ThirdPartyAPIISDown as e: logging.error(e) pass except BaseException: logging.exception("Unhandled failure") pass

That third party API is known to be flakey and we don’t need the full stack trace every time it’s unavailable.  Now we don’t have our own code swallowing issues and hiding some potential bugs.

Additional issues that may hide bugs include excessive complexity. This is a big topic and it straddles the third category, but ultimately overly complex code is hard to reason through and the combination of numerous logical paths and levels of these paths means very complex functions and methods are often where hidden bugs are to be found. The solution here is to refactor (in the “pure” sense of the word). This is sometimes easier said than done, and the urgency of refactoring complex code depends a lot on particularities about the module and it’s use.
 

Likely to cause developers to err

Pretty much everything else falls into this category. Style and formatting issues are at the least distractions and at worse can obscure what the code actually does.

These should be fixed but need not be top priority.

The good news, or better news, is that in many instances, these can be fixed automatically! Whether with a hard and fast formatter like black[0], a configurable one like autopep8[1], or a built-in tool like PyCharm[2], there are ways to address these without editing each extraneous space and each wayward tab individually. The further benefit of automated tools like these is that they can be added to your development process, and even automated build process, so that no one on the team has to think about them anymore.

Why _not _use an autoformatter? If your project has its own format guidelines or conventions you want to maintain, an existing autoformatter may either not work or require too much knob turning to be useful. 
 

The actual slicing and dicing

In our own work we’ve made use of some tooling that we recently published[3] to help break down the issues. The flake8-csv formatter exports the results from flake8 in CSV format with an option for including some pre-configured categorization. This can be loaded into a spreadsheet (or DataFrame!) letting you examine not only which modules have trouble spots but what the breakdown of issues looks like. 

Coupled with test coverage data and Git churn information, you can start to prioritize in even a large, busy codebase based on the most serious issues against the most frequently edited modules.

Indendtedly yours,
Ben

[0] Mentioned before, black is a Python3-only no-holds-barred formatter in the mold of the Go fmt command https://pypi.org/project/black/
[1] If you’d like more formatting options, autopep8: https://github.com/hhatto/autopep8
[2] PyCharm is a Python IDE and a commercial product, it can also be configured to format with an external tool of your choice https://www.jetbrains.com/pycharm/
[3] Flake8-csv https://pypi.org/project/flake8-csv/ Run flake8 with the –format=csv_categories flag to get the error code categorization.

Learn from more articles like this how to make the most out of your existing Django site.