Using an inner class for custom Manager in magic removal branch
Simon Willison
models in the magic-removal branch. Here's how it works at the moment:
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()
The above feels a little clumsy to me. Here's my proposed alternative:
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.
Basically, you define your own inner class called "Manager" if you want
to add custom methods to Person.objects (or modify the behaviour of
existing methods). If you don't define this inner class the default
Manager will be used.
The current system in magic-removal enables multiple managers (you can
assign as many as you like to different properties). I don't see the
advantage in allowing this - why not just add wrapper methods to your
custom Manager inner class, or even have it use multiple inheritance to
mix in methods from multiple classes?
Also up for debate is the way we handle the edge case where a model
needs to have a field called "objects". Currently we encourage people
to create their own Manager with a different name. I'm not keen on this
as it breaks user expectations - instead I think people should be
encouraged to do the following (and treat "objects" as a reserved
word):
class Person(models.Model):
objects_ = models.CharField(maxlength=30, db_column='objects')
Are there any problems with the above proposal that I've missed?
Further discussion is available on
http://code.djangoproject.com/ticket/1224
Russell Keith-Magee
>
> The current system in magic-removal enables multiple managers (you can
> assign as many as you like to different properties). I don't see the
> advantage in allowing this - why not just add wrapper methods to your
> custom Manager inner class, or even have it use multiple inheritance to
> mix in methods from multiple classes?
The only advantage to multiple managers that I can see is that having
multiple managers allows you to set up 'prefiltered' query sets. For
example, a Person class might have one manager (objects) that returns
queries over all people, but a second manager that adds a 'height >
180cm', so that 'Person.tall_people.get_list(name__exact='fred') will
retrieve all tall people named fred.
Granted; this is something that could be done by writing a whole lot
of get_tall_person_list class methods (and will require the developer
to write essentially the same code, just in a different place).
However, the tall_people.get_list() structure appeals to my
sensibilities.
I suppose the greater problem is the ability to easily construct (and
use) a filter on queries (e.g. construct a 'tall_people_only' filter,
make it a member of Person, so you can write
Person.get_list(tall_people_only, name__exact='Fred'). You should be
able to do this using arg-level query objects (see ticket #1133) - it
sounds like Robert's descriptor queries will make this sort of thing
even easier.
I'm a +0 on the multiple managers, downgrade to -0 with a good query
filter mechanism.
> as it breaks user expectations - instead I think people should be
> encouraged to do the following (and treat "objects" as a reserved
> word):
+1
Russ Magee %-)
oggie rob
use) a filter on queries (e.g. construct a 'tall_people_only' filter,
make it a member of Person, so you can write
Person.get_list(tall_people_only, name__exact='Fred'). You should be
able to do this using arg-level query objects (see ticket #1133) - it
sounds like Robert's descriptor queries will make this sort of thing
even easier.
Actually I think the problem is two-fold: the admin interface is too
good and the manipulator API is too poor!
I think trying to use multiple managers seems like a specific solution
to a broader problem. What happens when you need a manager to span
multiple models? It breaks down there.
I don't have any real alternative suggestions - sorry. I've been
extending the (0.91) admin interfaces as much as possible and my
biggest struggle has been these issues - things like limiting the
fields based on user, adding only certain fields from related models,
which differs between models, etc. I think the bigger issue is
divergance of manipulators and the admin managers. If these were more
interchangable (i.e. the Manager merely being a "shortcut" to dealing
with templates and manipulators, rather than a completely different
tool from manipulators) I don't think these issues would come up.
-rob
James Bennett
> Basically, you define your own inner class called "Manager" if you want
> to add custom methods to Person.objects (or modify the behaviour of
> existing methods). If you don't define this inner class the default
> Manager will be used.
I can see pros and cons to this, and I think the overriding question
should be how many use cases there are where you need a single manager
class to serve multiple models that don't inherit from each other. I'm
inclined to think there aren't enough to justify the separate manager
class, and so it'd be simpler and more logical to define the manger as
an inner class of the model, so I'd give it a tentative +1 barring any
compelling use cases for the current system.
> The current system in magic-removal enables multiple managers (you can
> assign as many as you like to different properties). I don't see the
> advantage in allowing this - why not just add wrapper methods to your
> custom Manager inner class, or even have it use multiple inheritance to
> mix in methods from multiple classes?
This one I'm not so sure about; I can certainly see the advantages of
multiple managers, but it'd also be awfully messy to have to define
and keep track of a whole string of inner manager classes on a model.
> Also up for debate is the way we handle the edge case where a model
> needs to have a field called "objects". Currently we encourage people
> to create their own Manager with a different name. I'm not keen on this
> as it breaks user expectations - instead I think people should be
> encouraged to do the following (and treat "objects" as a reserved
> word):
+1.
--
"May the forces of evil become confused on the way to your house."
-- George Carlin
hugo
>The above feels a little clumsy to me. Here's my proposed alternative:
[sample with inner class]
Uhm - in what way is nowadays defining classes and using them "clumsy"?
Thought that's what OO is all about. The beast is named "removing the
magic" for a good reason, so why throw in another "magic" (one that
turns a scoped class definition into a - differently named! -
attribute)? And why should we do a full round again on the discussion
about wether we want managers the way we already decided in the
previous round to have them? Actually this discussion sounds clumsy ;-)
Sorry, but I am -1 on this change. Several reasons:
- multiple managers make it possible to have different ways to access
your data. Don't just think "multiple managers to the same storage" as
it is now, think "multiple managers to different - maybe even
technically different - storages". One thing I can see done with
multiple managers would be a way to define managers that link to
specific databases and so have a model definition where you have one
manager attaching to your PostgreSQL database and another attaching to
your RDF storage.
- managers that are decoupled from the model class could be reused in
multiple models. For example I have a model with four quite similar
tables - even though they carry different payload, they do carry
similar basic structure for identification. They could definitely share
the same manager definition that would just do basic selecting (like
limiting to objects from the current site).
- constructed managers become possible due to the fact that
instantiation is explicit - you could have your manager class accepting
parameters in its __init__ and so parameterized a shared manager
definition (or used multiple but differently parameterized instances of
the same manager class).
Oh, and I strongly dislike the handling of an attribute named 'objects'
you propose. That's backwards into the discussion we already had. This
"you have to rename your attribute objects_ because we clobber your
namespace and won't allow you to reclaim it" felt like a hack to me
then and feels like a hack to me now. I am definitely -1 (it's more
like -2, if that would exist) on changing that to your proposal. The
current idea of explicitely instantiating and naming managers is much
cleaner.
Actually in simple cases you won't have to instantiate managers at all
- there will be a default manager created behind the scenes, IIRC. It's
just needed in those cases where you _do_ need explicit managers. And
one really typical scenario would be a situaiton where someone has a
basic relation in all his tables that should be filtered - like for
example ManyToMany relations to the Sites table. In that case you set
up one manager class that adds that filter and just use that manager
class in your models. This scenario would be much clumsier with having
to define a inner class - you would have to subclass the shared manager
class in every inner class of the model. And sorry, but stub inner
classes that magically instantiate attributes with a different name
just look clumsier to me than simple attribute assignements where you
use the exact same named attribute as you assigned ...
bye, Georg
mmarshall
One more point I would like to add:
- Flat is better than nested
Ok, so Tim Peters isn't infallible, but I think this is pretty good
advice.
my 2 cents
MWM
Simon Willison
On 16 Jan 2006, at 08:49, hugo wrote:
> Uhm - in what way is nowadays defining classes and using them
> "clumsy"?
> Thought that's what OO is all about. The beast is named "removing the
> magic" for a good reason, so why throw in another "magic" (one that
> turns a scoped class definition into a - differently named! -
> attribute)? And why should we do a full round again on the discussion
> about wether we want managers the way we already decided in the
> previous round to have them? Actually this discussion sounds
> clumsy ;-)
We already have the bit of magic in there that says "if no Manager is
defined, set one up and call it objects". This would just alter that
rule to include the condition "and use the inner class called Manager
if it exists".
> - multiple managers make it possible to have different ways to access
> your data. Don't just think "multiple managers to the same storage" as
> it is now, think "multiple managers to different - maybe even
> technically different - storages". One thing I can see done with
> multiple managers would be a way to define managers that link to
> specific databases and so have a model definition where you have one
> manager attaching to your PostgreSQL database and another attaching to
> your RDF storage.
OK, I'm convinced that multiple managers can be useful. There's
nothing about the inner class proposal however that would stop you
from using them:
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)
class RDFManager(django.rdf.Manager):
def get_list(self, **kwargs):
kwargs['limit'] = 10
return super(RDFManager, self).get_list(self, **kwargs)
objects = Manager() # This isn't necessary, it will happen
automatically if omitted
rdf_objects = RDFManager()
> - managers that are decoupled from the model class could be reused in
> multiple models. For example I have a model with four quite similar
> tables - even though they carry different payload, they do carry
> similar basic structure for identification. They could definitely
> share
> the same manager definition that would just do basic selecting (like
> limiting to objects from the current site).
That's possible with the inner class proposal as well:
class Person(models.Model):
first_name = models.CharField(maxlength=30)
last_name = models.CharField(maxlength=30)
Manager = MySpecialManagerClass
Or (based on the logic that objects is created only if it doesn't
already exist as a manager):
class Person(models.Model):
first_name = models.CharField(maxlength=30)
last_name = models.CharField(maxlength=30)
objects = MySpecialManagerClass()
That said, while there are workarounds for each of your objections,
put together they have convinced me that inner classes aren't nearly
as big a win as I thought they were. It also turns out you can use
them already if you want to just by explicitly instantiating them as
your objects property:
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)
objects = Manager()
That satisfies my desire for the stuff relating to the Person model
to live in the same class, and also means we don't have to add any
more magic to the framework.
> Oh, and I strongly dislike the handling of an attribute named
> 'objects'
> you propose. That's backwards into the discussion we already had. This
> "you have to rename your attribute objects_ because we clobber your
> namespace and won't allow you to reclaim it" felt like a hack to me
> then and feels like a hack to me now. I am definitely -1 (it's more
> like -2, if that would exist) on changing that to your proposal. The
> current idea of explicitely instantiating and naming managers is much
> cleaner.
Personally I find the idea of "every model class has an objects
property for manipulation in bulk, except when it doesn't" pretty
horrifying, especially from a code maintenance point of view. I guess
if people really want to make their lives more difficult we should
let them but I certainly wouldn't want it suggested as Django best
practise.
Overall then I think I'll withdraw the inner class proposal.
Cheers,
Simon
hugo
>property for manipulation in bulk, except when it doesn't" pretty
>horrifying, especially from a code maintenance point of view. I guess
>if people really want to make their lives more difficult we should
>let them but I certainly wouldn't want it suggested as Django best
>practise.
Actually I think the magic-removal way is quite good. It's some simple
steps where each step is bound to a specific need:
- by standard, the model class has an objects attribute that has a
default manager. This can be used to access data - and it's the most
simple case.
Now the specific need that's added is either one of those:
- you already have an attribute named "objects" - usually this will be
due to legacy database models
- you need more than one manager, with different behaviours
- you need just one manager, but it should behave differently from the
default (because you want to overload some of the bulk methods)
In any of those cases, you have to make managers explicit. So you
define a manager class and add it as an attribute to your model class.
I really don't see where this is "clumsy" :-)
One thing I would agree to is, the default 'objects' is magic that
should be removed. Actually that was my position in the discussion, to
make the manager assignment _allways_ explicit, so people allways will
choose the name of the manager object ;-)
bye, Georg
Russell Keith-Magee
>
> One thing I would agree to is, the default 'objects' is magic that
> should be removed. Actually that was my position in the discussion, to
> make the manager assignment _allways_ explicit, so people allways will
> choose the name of the manager object ;-)
+1. Gets around the reserved word problem entirely, and removes a
little bit more magic from model definitions.
Russ Magee %-)
hugo
>little bit more magic from model definitions.
Problem with that discussion was, IIRC, that neither Adrian nor Jacob
where very fond of allways having to explicitely provide the manager.
So the result of the discussion was what we now have in magic-removal -
a simple default way for the simple situations and an explicit way for
all situations that won't be solved by the default. I can live with
that.
bye, Georg
Robert Wittams
The real issue here is : what happens if it is left out? Some stuff
assumes a default manager ( eg any objects this is related to, the
admin, generic views, etc.) The only options then are
* Validation error, which just means that it becomes boilerplate - very
annoying
* Some kind of hidden default manager that is not kept in the model
class, but in a mapping somewhere else
Manager.get_default(MyModel) would get the first manager if defined in a
class, otherwise it will generate a default one and cache it somewhere.
This method would be used by the generic code.
I don't really have any opinion on which of these two options or the
current behaviour would be best...