#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 , 19 years ago
comment:2 by , 19 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 , 19 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 , 19 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:6 by , 19 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Following the discussion here: http://groups.google.com/group/django-developers/browse_thread/thread/9c160c88400ced9c/02c91e41f046c036
I'm withdrawing this proposal.
That above code example has a bug; it should look like this: