Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#1224 closed enhancement (wontfix)

Use inner class for custom Manager in magic removal branch

Reported by: Simon Willison Owned by: adrian
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: UI/UX:

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 Changed 9 years ago by 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.

comment:2 Changed 9 years ago by adrian

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 Changed 9 years ago by 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.

comment:4 Changed 9 years ago by adrian

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 Changed 9 years ago by Simon Willison

Shall do.

comment:6 Changed 9 years ago by Simon Willison

  • Resolution set to wontfix
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.
Back to Top