Use inner class for custom Manager in magic removal branch

[Simon Willison]

The current (implemented) proposal for adding custom table-level methods in RemovingTheMagic is a little clumsy:

class PersonManager(models.Manager):
    def get_list(self, **kwargs):
        # Changes get_list() to hard-code a limit=10.
        kwargs['limit'] = 10
        return models.Manager.get_list(self, **kwargs) # Call the "real" get_list() method.

class Person(models.Model):
    first_name = models.CharField(maxlength=30)
    last_name = models.CharField(maxlength=30)
    objects = PersonManager()

How about doing this using an inner class instead?

class Person(models.Model):
    first_name = models.CharField(maxlength=30)
    last_name = models.CharField(maxlength=30)
    class Manager(models.Manager):
        def get_list(self, **kwargs):
            # Changes get_list() to hard-code a limit=10.
            kwargs['limit'] = 10
            return self.get_list(self, **kwargs) # Call the "real" get_list() method.

Are there any reasons this couldn't work?

[Simon Willison]

That above code example has a bug; it should look like this:

class Person(models.Model):
    first_name = models.CharField(maxlength=30)
    last_name = models.CharField(maxlength=30)
    class Manager(models.Manager):
        def get_list(self, **kwargs):
            # Changes get_list() to hard-code a limit=10.
            kwargs['limit'] = 10
            return models.Manager.get_list(self, **kwargs) # Call the "real" get_list() method.

[Adrian Holovaty]

Using the inner-class technique, how would you give a model multiple managers? And if there were a field called "objects", how would you specify the manager's name?

[Simon Willison]

What is the use-case for multiple managers? Surely you can just add extra methods to the existing custom manager - and if you want to reuse components from other managers you can provide wrapper functions around them in the single manager class (or even use multiple inheritance).

I'm also not convinced that renaming the manager if a model includes a field called 'objects' is the right solution. It breaks user expectations and would break any code that assumed a manager called 'objects' (a generic pagination helper for example). The traditional way of solving this problem in other situations is to rename the field or variable to avoid clashing with the reserved word; examples include style.cssFloat = 'left' in JavaScript. Why not just tell people that if they have a DB column called 'objects' they have to do the following:

class Person(models.Model):
    objects_ = models.CharField(maxlength=30, db_column='objects')

That seems like a more elegant solution to me than changing the name of the manager field.

[Adrian Holovaty]

Both are great points. I'm convinced. Could you bring this up on django-developers just to make sure nobody else has concerns that we're overlooking?

[Simon Willison]

Shall do.

[Simon Willison]

Following the discussion here: ​http://groups.google.com/group/django-developers/browse_thread/thread/9c160c88400ced9c/02c91e41f046c036

I'm withdrawing this proposal.