On 19.01.2014, at 08:56, Russell Keith-Magee <russ...@keith-magee.com> wrote:
> Hi all,
>
> First off - this isn't critical for 1.7 alpha - I'm raising it now because
> I've been in this space for the last couple of days as a result of the
> working on the check framework. I suspect any changes stemming from this
> discussion can be landed in the beta period without major problems.
>
> Also - Aymeric - you've done a great job with this app loading work. I didn't
> get much of a chance to follow along as it was unfolding, but the resultant
> work has been a pleasure to work with.
>
> With one exception :-)
>
> We've now got AppConfig objects for each app (e.g., contrib.admin). Those
> AppConfig objects can have ready() methods that invoke startup logic. This is
> all great stuff that has been a long time coming.
>
> *But*
>
> From a backwards compatibility perspective, there seems to be a gap in the
> way AppConfig objects are instantiated.
>
> The gap is this - historical apps (and historical documentation) calls for
> admin to be put into INSTALLED_APPS using 'django.contrib.admin'. However,
> when you do this, you get the system default AppConfig, not something that is
> admin specific. In order to get the new AdminConfig, you need to update your
> INSTALLED_APPS to point at "django.contrib.admin.apps.AdminConfig".
>
> This is fine, but it limits the logic that we (as a project) can put into
> AdminConfig.ready(), because existing projects won't reference the new
> AdminConfig. So, cleanups like putting admin.autoregister and check framework
> registrations into the ready function can't be done, because existing
> projects won't automatically gain that functionality.
>
> So, we end up with a situation where we have a nifty new on-startup method,
> but we can't use it for any of our own on-startup app requirements, because
> we have no guarantees that it's going to be invoked.
>
Hi Russ, all,
Thanks for raising this issue as it’s indeed an important one to discuss. I
hoped we’d have time to let the community figure out some of the techniques to
handle version compatibility to Django but alas I didn’t expect the checks
framework to so thoroughly crash the party. Well done! :) I wish we would have
had more time to take these points into account when we were discussing the
various details back in December, but I guess as long as 1.7 isn’t out it’s not
too late. I probably would have made the argument that the check system may be
best placed in AppConfig classes (e.g. in AppConfig.check_* methods). But on
the other hand that may have made the work on the app config classes even
harder.
For the record, the app config concept has a long history with different
implementations and goals but always (AFAIR) tried to fix one thing: the side
effects of the implicit Django app concept. This issue is just another example
of that problem. It bubbles up in our compatibility strategy which is to still
allow dotted app module paths in INSTALLED_APPS. We assumed users to upgrade to
the dotted app config paths in INSTALLED_APPS on their own eventually if the
app in question required it. In other words, we really left it to the app
authors to decide when to ask their users to make the jump. Instead of a hard
coded check we wanted to gently push the new APIs into the community — also to
find concept holes.
I now believe that we may have been too conservative since that strategy
introduces a new level of uncertainty in the app API. I can see the user
questions already: “Does the 3rd party app XYZ work on 1.7?” “Can I also
install it on my legacy 1.6 project?” “How am I going to override some options
without a AppConfig?” “What’s the best way to create cross version compatible
apps?”
> I've dug through the mailing lists discussions about app-loading, and I can't
> see anywhere that this was explicitly discussed, so I can't tell if this was
> done deliberately, or as a side effect of the circular import problems that
> Aymeric described.
>
> Assuming that this wasn't done for import-related reasons, I can see a couple
> of possible solutions:
>
> 1) Require updating INSTALLED_APPS as a migration step. This is something we
> can detect with a check migration command - we inspect all the strings in
> INSTALLED_APPS; if it isn't an AppConfig subclass, look to see if there is an
> apps module, and if so, suggest that an update may be required.
>
> Benefits - explicit upgrade path.
> Downside - if you ignore the warning, you potentially lose functionality. And
> nobody ever reads the documentation :-)
+1 It’s the strictest and but also clearest migration path and I believe we
can’t afford to fix this over time and leave our users in limbo about how
Django apps work or what to expect from the app API. It means it’ll be harder
for reusable app developers to explain backward compatibility to their users
but we could offer some pointers, e.g. in blog/models.py::
try:
from django import apps
except ImportError:
from blog.utils import register_something
register_something()
That would work for Django versions < 1.7 and implies
blog.utils.register_something is called in the ready() method of
blog.apps.BlogConfig, too. It’s not an exciting pattern but at least common
enough for Python developers to require little explanation.
> 2) Use a naming convention - require that the default app configuration must
> be called AppConfig, so we can always try to load myapp.apps.AppConfig. If
> this object doesn't exist, install the system default AppConfig (as it does
> currently).
>
> Benefits - Easy to implement and document the requirement.
> Downside - It's coding by convention; also, every app config is called
> AppConfig, which is potentially painful for debugging.
Yeah, this is a bad idea. I love conventions, but not if they prevent me to
paint my shed blue. -1
> 3) Do 2, but with a level of indirection. Let the user call the AppConfig
> module whatever they want, but allow the apps module to define a
> 'default_app_config' attribute that provides the string to use as a default
> AppConfig.
>
> This is essentially halfway between 1 and 2 - it's implicitly doing the
> INSTALLED_APPS update, using information provided by the app developer.
While I understand that it would allow us to keep using dotted module paths in
INSTALLED_APPS, it’s also a great way to stall the adoption of app config
classes. To be clear: it *is* the goal to eventually have all apps (contrib and
3rd party) use app configs as the only location for startup code and app
configuration. App config classes are intended to be user and not only app
developer facing. We strive to encourage users to be app developers as soon as
possible, e.g. we tell them how to write an app in the tutorial. So introducing
a hidden configuration variable like ‘default_app_config’ makes me really
uncomfortable with regard to the mixed message we’d be sending. We should be
clear about what app is used. The “default app config” is the one that is
configured in the settings.py, just like the “default database backend” is the
one defined there. In other words it’s the users choice what is considered
default, not ours or the app developer’s.
> 3a) As for 3, but use a flag on the AppConfig subclass that marks it as "use
> me as the default". If there are more than one AppConfig objects in an apps
> module that has the 'use as default" flag, raise an error.
This smells like dependency tracking and hard to explain errors. I also don’t
see how a user could prevent 3rd party apps to not mark itself as default. -1
> 3b) As for 3, but use the first class discovered that is a subclass of
> AppConfig. Actually identifying the "first" is a bit of an implementation
> issue - I suppose we'd need to have a MetaClass that registered AppConfig
> objects as they are imported.
More implicit behavior that reminds me of how the model implementation made it
so hard to have pluggable user models. -1
Jannis
signature.asc
Description: Message signed with OpenPGP using GPGMail
