Opened 18 years ago

Closed 18 years ago

Last modified 17 years ago

#1224 closed enhancement (wontfix)

Use inner class for custom Manager in magic removal branch

Reported by: Simon Willison Owned by: Adrian Holovaty
Component: Database layer (models, ORM) Version: magic-removal
Severity: normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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?

Change History (6)

comment:1 by Simon Willison, 18 years ago

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.

comment:2 by Adrian Holovaty, 18 years ago

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?

comment:3 by Simon Willison, 18 years ago

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.

comment:4 by Adrian Holovaty, 18 years ago

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?

comment:5 by Simon Willison, 18 years ago

Shall do.

comment:6 by Simon Willison, 18 years ago

Resolution: wontfix
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top