ISTM this would solve the "auth.User" issue, but doesn't help reusable
apps at large: one can trivially imagine a project that wants voting
(or tagging ;), or commenting, or ...) on more than one model.
In any event, my brain needs to digest (and I need lunch),
Alex
--
"I disapprove of what you say, but I will defend to the death your
right to say it." -- Voltaire
"The people's good is the highest law." -- Cicero
"Code can always be simpler than you think, but never as simple as you
want" -- Me
On first inspection, absent of an implementation, I think this is an
interesting approach.
My biggest technical concern is the same as Alex's -- that it doesn't
address the 'FK to multiple models' problem. While I agree with your
'no silver bullet' response to Alex, I also don't want to end up with
two (or more) completely different ways of solving the same problem.
At the very least, I'd like to have some certainty that the solution
for single concrete class problem will be conceptually similar to the
multiple concrete class problem.
I also have two technical concerns about how the virtual keyword will
work in practice.
Firstly, take the likely migration path for contrib.auth. We would
introduce an AbstractUser that encompasses the 'basic' concept of a
user, and is marked as a virtual. But we still need to ship a concrete
User that provides the current implementation. At which point, we now
have our single allowed concrete instantiation, and users can't define
their own User class.
I can see this being a common pattern; if you want your app to work
out of the box, you will provide a bare-bones concrete implementation,
which will then block anyone else from providing a concrete
implementation. This necessitates introducing either the ability to
hide a model, or provide a different way of registering models so that
unneeded concrete types aren't instantiated.
Of course, the simple solution here would be to split the concrete
model out into a different app, so that you optionally include
auth.User if you actually want it.
Secondly, this approach requires that content objects that are to be
the target of these relationships must share a virtual base class.
This makes for great pure-OO, but it sucks from the point of view of
duck typing.
For example, consider the case of tagging -- if you want to put a Tag
in a relationship with some content object, then you need to make that
content object inherit from a virtual "TaggableObject" base class.
This means that you need to have control of the base class so that you
can install that mixin. If you're using a reusable app from a third
party to provide your content object (e.g., a Blog model from a
blogging app), you don't have access to the model definition.
The rest of this email is thinking out loud, and probably has a whole
bunch of sharp edges.
It seems to me that what we need isn't a 'virtual' keyword on the
content class, but a virtual representation on the referrer class,
plus a way of registering instances of concrete subclasses.
Here's how it might work:
* Allow a model to have a ForeignKey to an abstract base class; but
in doing so, you make the model with the FK a virtual model.
* Introduce a "VirtualForeignKey" that can point at *any* content
object. Again, having a VirtualForeignKey on a model makes the entire
model virtual.
* In configuration code (I'm a little hazy on exactly where would be
best), provide a way to instantiate virtual models as concrete models.
So, since blog entries can be owned, we would need to register:
MyBlogEntry = make_concrete("MyBlogEntry", model=BlogEntry, owner=MyUser)
So - when we want to allow blogs to be tagged, we might register:
BlogTag = make_concrete("BlogTag", model=Tag, content=MyBlogEntry)
Conceptually, a model could even have multiple virtual extension points:
BlogTag = make_concrete("BlogTag", Tag, content=MyBlogEntry,
image=PrettyPicture)
As a result of these changes, code like:
Blog.objects.all()
wouldn't work out of the box, because Blog is a virtual class.
However, you could use the app cache:
MyBlogEntry = get_model('blog','MyBlogEntry')
MyBlogEntry.objects.all()
We could also include a shortcut on model classes to find their
concrete instances:
Blog.concrete(owner=MyUser).objects.all()
In practice, this would mean that reusable apps that had virtualized
components will need to be careful to ensure that the concrete model
is appropriately realized, and that the concrete model is then passed
around the app as required. For example, Django's admin (in a
virtualized User world) would need to make all the internal models
with FKs to user were rendered concrete.
This also fits in nicely with the app-cache refactor, because the
'app' object provides a convenient place to specify models and
perform the concreting process.
This would also be backwards compatible, because FKs to abstract base
classes are forbidden at the moment. This new behaviour would only be
required for models that introduced virtualizing foreign keys.
Again - this isn't completely thought through, and I have written even
less code than you have :-), but I'm putting it out on the stoop to
see if the cat licks any of it up.
Yours,
Russ Magee %-)
I seem to gradually be going away from GenericForeignKey and using
"Glue"-models instead. App1, model1, is connected to App2, model2 via
App3, Glue-model3, which has foreign-keys to model1 and model2, and
sometimes a little extra. App1 and App2 need not know about eachother
at all that way.
I'm considering making my own tagging-module this way, as tags *is*
one of the reusable apps that is useful to connect to more than one
model in a project. Ditto for comments.
HM
With the risk of being ignored once again, I dare to link to a working
solution that does not need any changed to the framework itself (other
than perhaps including the factory class):
--
Patryk Zawadzki
>
> With the risk of being ignored once again, I dare to link to a working
> solution that does not need any changed to the framework itself (other
> than perhaps including the factory class):
>
> http://gist.github.com/584106
This looks rather good to me. It may have been ignored before because
it has no comments and some things are not immediately obvious. For
example, you are basically proposing that the concrete models are passed
into view functions via URLconf, and from there are passed into any
functions which need them, and so they would never actually need to be
imported by the app that defines the abstract model.
I for one would be much happier to not add any more machinery via Meta
options. With some cleanup, and some documentation of this pattern, and
possibly a better name, I think the AbstractMixin class you propose
could be a good candidate for inclusion in core.
Some notes:
1) it seems like line 15 in abstract.py should say 'abstract':'False',
not 'True' - did I miss something?
2) there would need to be some way of merging the concrete class's own
Meta options with the abstract class's Meta options
3) why do we need the _classcache? Is the key used specific enough -
what if two different apps both create 'MyCategory' based on
'CategoryFactory', using them in different situations?
Thanks,
Luke
--
"Christ Jesus came in to the world to save sinners" (1 Timothy 1:15)
Luke Plant || http://lukeplant.me.uk/
> Some notes:
> 1) it seems like line 15 in abstract.py should say 'abstract':'False',
> not 'True' - did I miss something?
Cancel that one, I was missing the fact that we are still inheriting
from the generated class, not doing:
MyCategory = CategoryFactory.construct()
It is there because I inherit from the .construct() result rather than
taking it directly as a solid class. This helps with debugging as all
the tracebacks show proper module for the model.
> 2) there would need to be some way of merging the concrete class's own
> Meta options with the abstract class's Meta options
True.
> 3) why do we need the _classcache? Is the key used specific enough -
> what if two different apps both create 'MyCategory' based on
> 'CategoryFactory', using them in different situations?
I did it so there was no way to create two different mechanics for the
same model accidentally.
--
Patryk Zawadzki