ModelAdmin and AlreadyRegistered
Brian Rosner
behavior of AdminSite throwing the AlreadyRegistered exception. More
and more people are using newforms-admin and I am beginnging to see
where people are getting tripped up. This maybe simply be a
documentation problem as there really isn't much on newforms-admin.
Here are a couple options I think we should consider:
1. Remove the exception and allow subsequent registrations of
ModelAdmin classes to simply update the registry. This allows for user
code to override ModelAdmins in django.contrib or other third party
application. That could also be accomplished with just defining your
own site, but that path for many beginners may be more difficult unless
documented well.
2. Throw the exception based on whether the ModelAdmin is different
from a previously registered one. This will technically keep the
current behavior, but is really special casing the instances where
admin.site.register is called more than once. This currently happens
every so often with the models.py which is the documented method of
registration.
3. Keep current behavior and let documentation take care of it.
I personally am in favor of #1, but the only problem I have with it is
in the case of unintended side-effects that may result in much more
difficult debugging. What are everyone else's thoughts?
--
Brian Rosner
http://oebfare.com
Malcolm Tredinnick
On Fri, 2008-02-29 at 10:57 -0700, Brian Rosner wrote:
> I would like to ignite some discussion about removing or changing the
> behavior of AdminSite throwing the AlreadyRegistered exception. More
> and more people are using newforms-admin and I am beginnging to see
> where people are getting tripped up. This maybe simply be a
> documentation problem as there really isn't much on newforms-admin.
>
> Here are a couple options I think we should consider:
>
> 1. Remove the exception and allow subsequent registrations of
> ModelAdmin classes to simply update the registry. This allows for user
> code to override ModelAdmins in django.contrib or other third party
> application. That could also be accomplished with just defining your
> own site, but that path for many beginners may be more difficult unless
> documented well.
This is a little unstable because it depends on the order of imports.
Once we get into a battle of "this must go ahead of this" in
INSTALLED_APPS, it's a pretty slippery slope. Plus, imports happen at
reasonably hard to predict times (the ordering can be different to what
you expect) in any case, due to the app_cache. So I'd try to avoid this
path.
> 2. Throw the exception based on whether the ModelAdmin is different
> from a previously registered one. This will technically keep the
> current behavior, but is really special casing the instances where
> admin.site.register is called more than once. This currently happens
> every so often with the models.py which is the documented method of
> registration.
This would seem like the obvious approach, providing there's a way to
compare two ModelAdmin subclasses for equality. It doesn't even have to
be a particularly cheap/fast test, since it doesn't happen that often.
>
> 3. Keep current behavior and let documentation take care of it.
People will be bitten by different import paths. It's far too easy to
have both "app" and "project.app" imported without realising why (or any
number of variations on this theme). We get bitten by this all over the
place in Django and it's to be avoided if at all possible.
Malcolm
--
Monday is an awful way to spend 1/7th of your life.
http://www.pointy-stick.com/blog/
Jacob Kaplan-Moss
say, list_display for some contrib app from time to time. Yes, it
smacks a bit of monkeypatching, but still... I gotta say I like it.
I'm +0 on option 2, too, so really either one's OK.
Jacob
Malcolm Tredinnick
On Fri, 2008-02-29 at 13:09 -0600, Jacob Kaplan-Moss wrote:
> I actually like option 1 quite a bit -- I've often wanted to tweak,
> say, list_display for some contrib app from time to time. Yes, it
> smacks a bit of monkeypatching, but still... I gotta say I like it.
I wonder if that's mixing up two problems: accidental re-registration
and intentional overriding. A possible solution is that intentional
overriding passes in a parameter to the registration method to say that
it's expected/permitted to override anything that is already registered.
On the other hand, I don't think removing the error check is going to
hurt much either for most people. There's nowhere else in a language
like Python (or C or Perl or ...) where you have to pre-announce "I'm
going to be using your name now." and the originally scoped variable
gets to complain and veto.
Malcolm
--
If Barbie is so popular, why do you have to buy her friends?
http://www.pointy-stick.com/blog/
Michael Newman
because I am getting bitten by it again right now. This problem is
created frequently by programs that have multiple relationships
between models. Also if there is any programming that exists in the
__init__, or model beyond just importing admin, that code is
completely inaccessible. Currently when an AlreadyRegistered error is
raised, it come back as being an import error, for me, so deep in the
program that I don't understand why my blog model is pulling my
profile model only to find that the relationship is so elemental that
it is impossible to trackback short of unstringing the entire
application. This is a frustration that has gotten me two times right
at deadline.
An example of a model that would be too difficult to unravel to put an
import admin into any of the usual places right now is the
contrib.auth. http://code.djangoproject.com/ticket/6083 this ticket
tries to get auth into the newforms, but where should I put the import
admin. The model needs to be called several times the init is a core
function of code. The only solution I have found is placing the import
django.contrib.auth.admin in my own models, that are simple enough to
only be called once.
I don't think the errors should be ignored or overwritten as they are
being imported because that seems like there could be a major flaw in
logic. Instead I think that maybe we should look into having admin try
to import admin.py in each app of a project, like the way templatetags
are imported. That might be a little too much magic. Another solution
that might work is adding a new Setting that works in tandem with the
installed apps setting to import admin.py.
Let me know what you think,
Michael
On Mar 1, 3:42 am, Malcolm Tredinnick <malc...@pointy-stick.com>
wrote:
Michael Newman
__init__:
from django.conf import settings
for a in settings.INSTALLED_APPS:
try:
__import__(a + '.admin')
except ImportError:
pass
Which will make my life a lot easier. I can't say I have thought about
every possible problem, but having it so that when someone puts
contrib.admin in their installed apps and django automatically starts
looking for admin.py in installed apps sounds good to me.
On Mar 1, 4:40 pm, Michael Newman <newmani...@gmail.com> wrote:
> I actually just logged in to come back and talk about this issue,
> because I am getting bitten by it again right now. This problem is
> created frequently by programs that have multiple relationships
> between models. Also if there is any programming that exists in the
> __init__, or model beyond just importing admin, that code is
> completely inaccessible. Currently when an AlreadyRegistered error is
> raised, it come back as being an import error, for me, so deep in the
> program that I don't understand why my blog model is pulling my
> profile model only to find that the relationship is so elemental that
> it is impossible to trackback short of unstringing the entire
> application. This is a frustration that has gotten me two times right
> at deadline.
>
> An example of a model that would be too difficult to unravel to put an
> import admin into any of the usual places right now is the
Russell Keith-Magee
>
> I would like to ignite some discussion about removing or changing the
> behavior of AdminSite throwing the AlreadyRegistered exception. More
> and more people are using newforms-admin and I am beginnging to see
> where people are getting tripped up. This maybe simply be a
> documentation problem as there really isn't much on newforms-admin.
I've hit this error a few times myself, but I haven't had a chance to
dig into the code to find the exact cause and the right solution.
However, my preliminary investigations suggested that this was a
variant on the multiple registration problems we used to have before
the model cache was refactored. If this is the case....
> Here are a couple options I think we should consider:
>
> 1. Remove the exception and allow subsequent registrations of
> ModelAdmin classes to simply update the registry. This allows for user
> code to override ModelAdmins in django.contrib or other third party
> application. That could also be accomplished with just defining your
> own site, but that path for many beginners may be more difficult unless
> documented well.
This sounds like a recipe for disaster to me. The end result - being
able to extend a ModelAdmin definition - is potentially useful, but
the unpredictability of registration order means that we won't really
be able to easily control (or document) whether ModelAdmin A extends
ModelAdmin B, or vice versa.
> 2. Throw the exception based on whether the ModelAdmin is different
> from a previously registered one. This will technically keep the
> current behavior, but is really special casing the instances where
> admin.site.register is called more than once. This currently happens
> every so often with the models.py which is the documented method of
> registration.
+1 to this option. We need to prevent users from registering a model
with an admin site multiple times, but it is possible for a single
site.register to get invoked multiple times as a result of the import
process. Checking if the registration has changed seems like a good
way to achieve this, and is somewhat analogous to the way the model
cache extracts the app.model name from a model definition and using
that as the basis for eliminating duplicates.
> 3. Keep current behavior and let documentation take care of it.
If I have correctly understood the errors I was getting, this isn't
really something that documentation can (or should) fix, because there
are perfectly valid Python import statements that will cause
difficulty for the admin registration process.
Yours,
Russ Magee %-)
Rajeev J Sebastian
>
> To elaborate I just threw 4 lines of code into my contrib.admin
> __init__:
> from django.conf import settings
> for a in settings.INSTALLED_APPS:
> try:
> __import__(a + '.admin')
> except ImportError:
> pass
> Which will make my life a lot easier. I can't say I have thought about
> every possible problem, but having it so that when someone puts
> contrib.admin in their installed apps and django automatically starts
> looking for admin.py in installed apps sounds good to me.
Regards
Rajeev J Sebastian
Michael
Unless I am missing something, I don't see where this is being done. The installed models are run through that right now the system involves importing the admin in that situation. I am still hitting error after error in my app that things are already registered due to relationships inside my models that I don't see how to avoid.
I think that the second solution that Brian proposed is the best solution, but, to me, it sounds strange to accept the fact that a model might be imported twice and ignored the second time. The admin only needs to be imported once, so I don't understand why it should be in an import statement in code that might need to be executed several times.
Please guide me where I am wrong,
Thanks, Michael
Rajeev J Sebastian
I guess my very terse response caused some misunderstanding.
I meant, we (i.e., dinamis.com in our inhouse projects) put all our
admin related stuff for each app in an <app>/admin.py and load it at
startup.
Sorry for the misunderstanding I caused.
Regards
Rajeev J Sebastian