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.
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?
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.
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'm +0 on option 2, too, so really either one's OK.
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.
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
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:
> 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.
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.
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
> contrib.auth.http://code.djangoproject.com/ticket/6083this 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:
> > 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.
On Sat, Mar 1, 2008 at 2:57 AM, Brian Rosner <bros...@gmail.com> 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.
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.
On Sun, Mar 2, 2008 at 3:29 AM, Michael Newman <newmani...@gmail.com> wrote:
> 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.
> On Sun, Mar 2, 2008 at 3:29 AM, Michael Newman <newmani...@gmail.com> > wrote:
> > 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. > We already do this.
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.
On Sun, Mar 2, 2008 at 8:43 PM, Michael <newmani...@gmail.com> wrote:
> On Sun, Mar 2, 2008 at 9:34 AM, Rajeev J Sebastian > <rajeev.sebast...@gmail.com> wrote:
> > On Sun, Mar 2, 2008 at 3:29 AM, Michael Newman <newmani...@gmail.com> > wrote:
> > > 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. > > We already do this.
> 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.
> 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
> On Sun, Mar 2, 2008 at 8:43 PM, Michael <newmani...@gmail.com> wrote:
> > On Sun, Mar 2, 2008 at 9:34 AM, Rajeev J Sebastian > > <rajeev.sebast...@gmail.com> wrote:
> > > On Sun, Mar 2, 2008 at 3:29 AM, Michael Newman <newmani...@gmail.com> > > wrote:
> > > > 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. > > > We already do this.
> > 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.
