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.
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 :-)
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.
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.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.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.
All of your proposed solutions require trying to import an apps.py module which may not exist. I know Aymeric was very much against this, especially as python imports have potential side effects, and we don't know what people might have in a apps module already.
Anyways, configuration over convention and explicit better than implicit etc.
It does mean that we can't put any required logic into AppConfig, but for example admin.autodiscover is fine as existing projects already call it in their urls.py (or wherever they moved it to), so there is no concern here, especially if we update the project template.
The only other obvious contrib thing which would be nice to move is signal registration, but this isn't that necessary.
The hope is that people will migrate towards using AppConfig in their settings, and then we can reconsider. Third party apps who wish to do a lot of logic there may wish to require it.
Personally I'm torn, as I can see the benefits of a convention. But equally I don't think there's a huge amount of point making a big effort to rewrite Django's import handling in order to add more magic back in.
Marc
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAJxq849Hx24ETnugK54xGGRpQNUFON18YSwPdXKv0fYg%2B7tnxQ%40mail.gmail.com.
For more options, visit https://groups.google.com/groups/opt_out.
On 19 janv. 2014, at 08:56, Russell Keith-Magee <rus...@keith-magee.com> wrote:
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.I would take a more moderate position.We can certainly use AppConfig.ready() for new features such as the checks framework. It’s reasonable to ask our users to edit their configuration to take advantage of a new feature. If they don’t, they don’t lose anything compared to the previous version of Django. Backwards-compatibility is preserved.
We can also use it for optional features such as admin.autodiscover(). Putting it in urls.py still works, but it’s simply better to use AdminConfig. The release notes explain the upgrade path. It users don’t follow them, they don’t lose anything.
If we wanted to move a major existing feature to AppConfig.ready(), and stop supporting the previous implementation, then we’d have to implement a deprecation path or, depending on the context, document it as a backwards-incompatible change. But this situation still has to happen.
As far as I can tell, we’re talking about an hypothetical problem.
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 :-)It really depends how much we want to push the use of AppConfig. At the moment the consensus among the core devs I’ve spoken with is “wait and see”. That means not making the feature too prominent in 1.7 and adding suitable documentation in 1.8, depending on how well it works.
Also there might be good reasons for not using an AppConfig.
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.Jannis vetoes that idea and I believe he’s right. Auto-importing modules for discovery is a bad idea. “One does not simply import modules.” ;-)
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.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.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.All these still suffer from the implicit import problem. I chose not to perform any kind of discovery because explicit > implicit
To avoid that, we could put default_app_config in the application’s root package but that’s ugly.
All of your proposed solutions require trying to import an apps.py module which may not exist. I know Aymeric was very much against this, especially as python imports have potential side effects, and we don't know what people might have in a apps module already.
Anyways, configuration over convention and explicit better than implicit etc.
It does mean that we can't put any required logic into AppConfig, but for example admin.autodiscover is fine as existing projects already call it in their urls.py (or wherever they moved it to), so there is no concern here, especially if we update the project template.
The only other obvious contrib thing which would be nice to move is signal registration, but this isn't that necessary.
The hope is that people will migrate towards using AppConfig in their settings, and then we can reconsider. Third party apps who wish to do a lot of logic there may wish to require it.
Personally I'm torn, as I can see the benefits of a convention. But equally I don't think there's a huge amount of point making a big effort to rewrite Django's import handling in order to add more magic back in.
For what it's worth, if we have some code that:
a) is a new feature we want to ensure runs
b) has no obvious, explicit place to place it
and
c) is trivially solved by .ready()
Then I start to think that practicality beats purity. Personally, I think we should still encourage the "explicit" approach where we recommend to users that they use 'path.to.AppConfig' and maybe one day deprecate not having an AppConfig - for external apps at least this ensures translatable admin, but in the mean time add an impure convention (which if we are going to do, I don't care which one we do).
Explicit > Implicit yes, but if our choice is 'introduce an implicit convention, which we could one day deprecate' vs 'add some more import time side effect code' then I personally think we should go for the former.
In Django 2.0 we do away with all the implicit importing and searching and you have to configure your AppConfig instances for management commands, template directories, static dirs, models, admin config... ;)
Marc
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAJxq84_MPKw_zywaeaQ-4aXDYaTVJxfRtm%2B-2GEXrrn7D7VwJQ%40mail.gmail.com.
For what it's worth, if we have some code that:
a) is a new feature we want to ensure runs
b) has no obvious, explicit place to place it
and
c) is trivially solved by .ready()Then I start to think that practicality beats purity. Personally, I think we should still encourage the "explicit" approach where we recommend to users that they use 'path.to.AppConfig' and maybe one day deprecate not having an AppConfig - for external apps at least this ensures translatable admin, but in the mean time add an impure convention (which if we are going to do, I don't care which one we do).
Explicit > Implicit yes, but if our choice is 'introduce an implicit convention, which we could one day deprecate' vs 'add some more import time side effect code' then I personally think we should go for the former.
In Django 2.0 we do away with all the implicit importing and searching and you have to configure your AppConfig instances for management commands, template directories, static dirs, models, admin config... ;)
On Sun, Jan 19, 2014 at 5:48 PM, Aymeric Augustin <aymeric....@polytechnique.org> wrote:
We can certainly use AppConfig.ready() for new features such as the checks framework. It’s reasonable to ask our users to edit their configuration to take advantage of a new feature. If they don’t, they don’t lose anything compared to the previous version of Django. Backwards-compatibility is preserved.I'm afraid I don't see *how* we can use ready() for new features. Checks are *definitely* off the table for using the feature.
Also there might be good reasons for not using an AppConfig.I'd be interested in hearing what these are.
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).
Jannis vetoes that idea and I believe he’s right. Auto-importing modules for discovery is a bad idea. “One does not simply import modules.” ;-)
I'll certainly appreciate (and largely agree with) that logic. My concern is the pragmatic one of not being able to use ready() as a clean approach to a problem I'm currently solving in a less than ideal way.
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.
we could put default_app_config in the application’s root package but that’s ugly.I agree that it's less than ideal, but I don't think a string definition in __init__is *more* ugly than implicit imports and registrations happening in the __init__ - in fact, I'd say it's a whole lot *less* ugly, because it's just identifying a string in a module that was already being imported, and using that as a trigger to import another specific module (one that the end user could reference explicitly if they were so inclined).
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/45F925BB-699F-4D7D-9871-ACD766E85926%40polytechnique.org.
On 01/20/2014 01:01 PM, Carl Meyer wrote:
>>> 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
>
> I don't see what advantage this has over (3) - it just allows another
> failure mode. -0.
I just realized that this option also requires adding an automatic
attempt to import the "apps" submodule within an app, when the "apps"
submodule was not explicitly referenced in settings (effectively making
the "apps" submodule a magic name, and potentially causing immediate
issues for apps that already have an apps.py for whatever reason.)
So count me as -1 on this option, not -0. (And I'd be similarly -1 on
the "default_app_config", if it were required to be defined in apps.py
rather than __init__.py, for the same reason. But I'm still +1 on it if
it's defined in __init__.py.)
Carl
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/52DD8289.9010808%40oddbird.net.
On 19.01.2014, at 08:56, Russell Keith-Magee <rus...@keith-magee.com> wrote:Hi Russ, all,
> 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.
>
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.+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::
>
> 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 :-)
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).Yeah, this is a bad idea. I love conventions, but not if they prevent me to paint my shed blue. -1
>
> 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.
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.
> 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.