Code Kills

The dangers of Python's super have been documented... but, in my humble opinion, not well enough.

A major problem with Python's super() is that it is not straight forward to figure out needs to call it, even if it doesn't seem like the method's parent class should need to.

Consider this example, where mixins are used to update a dictionary with some context (similar to, but less correct than, for example, Django's TemplateView):

class FirstMixin(object):
    def get_context(self):
        return {"first": True}

class BaseClass(FirstMixin):
    def get_context(self):
        ctx = super(BaseClass, self).get_context()
        ctx.update({"base": True})
        return ctx

class SecondMixin(object):
    def get_context(self):
        ctx = super(SecondMixin, self).get_context()
        ctx.update({"second": True})
        return ctx

class ConcreteClass(BaseClass, SecondMixin):
    pass

This looks correct... but it isn't! Because FirstMixin doesn't call super(), SecondMixin.get_context is never called:

>>> c = ConcreteClass()
>>> c.get_context()
{"base": True, "first": True} # Note that ``"second": True`` is missing!

Alternatively, image that FirstMixin.get_context() does call super():

class FirstMixin(object):
    def get_context(self):
        ctx = super(FirstMixin, self).get_context()
        ctx.update({"first": True})
        return ctx

This will also be incorrect, because now the call to super() in SecondMixin will trigger an error, because the final base class - object - does not have a get_context() method:

>>> c = ConcreteClass()
>>> c.get_context()
...
AttributeError: 'super' object has no attribute 'get_context'

What is a poor Pythonista to do?

There are three reasonably simple rules to follow when dealing with this kind of multiple inheritance:

1. Mixins should always call super().
2. The base class should not call super().
3. The base class (or one of its super classes) needs to be at the right of sub-classe's list of base classes.

Note that this will often mean introducing an otherwise unnecessary *Base class.

To correct the example above:

# Following rule (1), every mixin calls `super()`
class FirstMixin(object):
    def get_context(self):
        ctx = super(FirstMixin, self).get_context()
        ctx.update({"first": True})
        return ctx

# Following rule (2), the base class does *not* call super.
class BaseClassBase(object):
    def get_context(self):
        return {"base": True}

# Notice that, to follow rule (3), an otherwise uneccessary base class has
# been introduced to make sure that the "real" base class (the one without
# the call to super) can be at the very right of the list of base classess.
class BaseClass(FirstMixin, BaseClassBase):
    pass

# Following rule (3), the base class comes at the right end of the list
# of base classess.
class ConcreteClass(SecondMixin, BaseClass):
    pass

This will guarantee that the mixins are always called before the base class, which doesn't call super() in get_context().

Note that this will still cause problems in the even that multiple base classess are used (ie, "true" multiple inheritance)... and there isn't much which can be done about that, at least in the general case.

It is also worth noting that that in many cases the best solution is to avoid inheritance all together, opting instead for a pattern better suited to the requirements of the specific problem at hand.

For example, in the sitaution from the example above - where many different "things" (in the above example: mixins and the base class) need to contribute to the "context" dictionary - one option which might be more appropriate is an explicit set of "context providers":

class FirstContextProvider(object):
    def __call__(self):
        return {"first": True}

class BaseClass(FirstMixin):
    context_providers = [
        FirstContextProvider(),
        lambda: {"base": True},
    ]

    def get_context(self):
        ctx = {}
        for provider in self.context_providers:
            ctx.update(provider())
        return ctx

class SecondContextProvider(object):
    def __call__(self):
        return {"second": True}

class ConcreteClass(BaseClass, SecondMixin):
    context_providers = BaseClass.context_providers + [
        SecondContextProvider(),
    ]

(recall that __call__ method is used to make instances of a call callable)

Edit: I was corrected by @lambacck, who pointed out the "base class on the right" rule: https://twitter.com/lambacck/status/451528854507905024

Often test cases require a few stateful "things" to happen during each run, which are often configured in the setUp and tearDown methods of the test case.

For example, some common "things" in applications I've worked on are:

• Capturing email messages sent during the test case
• Capturing log messages emitted during the test case
• Mocking Redis connections
• Mocking the application's job queue

And the implementation often looks something like this:

class MyRegularTestCase(TestCase):
    def setup(self):
        self.mail = setup_mock_mail()
        self.logs = setup_mock_logger()
        self.redis = setup_mock_redis()
        self.jobs = setup_mock_job_queue()

    def teardown(self):
        self.mail.teardown()
        self.logs.teardown()
        self.redis.teardown()
        self.jobs.teardown()

    def test_foo(self):
        foo()
        self.mail.assert_message_sent("hello, world")
        self.logs.assert_logged("hello, world")
        self.redis.assert_list_contains("foo", "bar")
        self.jobs.assert_job_queued("some_job")

And that is a best case example; most of the time test code doesn't use high-level mocking objects, and setup code is copy+pasted between test classes.

This makes me wonder: why aren't composable test helpers a thing?

For example, a log capturing test helper might look something like this:

class LogCapture(object):
    def __init__(self, logger_name=''):
        self.logger = logging.getLogger(logger_name)

    def setup(self):
        self.records = []
        self.logger.addHandler(self)

    def teardown(self):
        self.logger.removeHandler(self)

    def emit(self, record):
        self.records.append(record)

    def assert_logged(self, message):
        for record in self.records:
            if message in record.getMessage():
                return
        raise AssertionError("No log message containing %r was emitted" %(message, ))

And this sort of composable helper could be used with a TestCase like this:

class MyBetterTestCase(ComposableTestCase):
    mail = MailCapture()
    logs = LogCapture()
    redis = MockRedis()
    jobs = MockJobQueue()

    def test_foo(self):
        foo()
        self.mail.assert_message_sent("hello, world")
        self.logs.assert_logged("hello, world")
        self.redis.assert_list_contains("foo", "bar")
        self.jobs.assert_job_queued("some_job")

I'll be building this kind of composable test helper into my application, and if it works well, I'll release a library.

Consider South's orm object, http://south.readthedocs.org/en/latest/ormfreezing.html, which provides "frozen-in-time" versions of ORM models:

(Pdb++) from my_app.models import MyModel
(Pdb++) MyModel
<class 'my_app.models.MyModel'>
(Pdb++) orm['my_app.MyModel']
<class 'my_app.models.MyModel'>
(Pdb++) orm['my_app.MyModel'] == MyModel
False

Notice that the magic South frozen model appears to be in the same namespace as my real model: my_app.models!

This is at best stupid, and at worst potentially dangerous.

It makes perfect sense that South needs some mechanism for providing "frozen models" (ie, which match the database schema at the time of migration, of the current schema), but they are emphatically not the same as the real models (don't have any of the real methods, etc), so it should be made very clear that they are different! There are many ways this could be done, but one very simple step would be putting them in a different namespace, which makes it clear that these models are frozen, and references the migrations they belong to:

(Pdb++) orm['my_app.MyModel']
<class 'south.frozen_models.my_app.models_0003.MyModel'>

Additionally, this is a bad code smell which could potentially lead to bugs: because the "frozen model class" is lying about where it's from, any code which relies on the module path will do surprising things. In fact, it's such a big problem that pickle explicitly checks for this case:

(Pdb++) import pickle
(Pdb++) pickle.dumps(orm['my_app.MyModel'])
*** PicklingError: Can't pickle <class 'my_app.models.MyModel'>:
    it's not the same object as my_app.models.MyModel

An anti-pattern I've often seen is using try/except incorrectly in an attempt at duck typing:

try:
    widget.frob()
except AttibuteError:
    # widget can't be frobbed
    do_something_else()

This is fundamentally broken because it will conceal bugs in the widget.frob() method (ie, if there's a bug in widget.frob() which causes an AttibuteError to be raised).

In this situation, the only safe way to use try/except would be:

try:
    widget_frob = widget.frob
except AttibuteError:
    # widget can't be frobbed
    do_something_else()
else:
    widget_frob()

This ensures that bugs in widget.frob() won't be accidentally hidden.

Of course, this code is no where near as pretty as the "incorrect" version, which is why I've come to believe that using try/except for duck typing could be considered an anti-pattern, and that hasattr is generally preferable (except in situations where performance is important, as try/except will almost certainly be faster):

if hasattr(widget, "frob"):
    widget.frob()
else:
    # widget can't be frobbed
    do_something_else()

And of course, all this goes without saying that a naked except: is always an absolutely terrible idea...