Merged: https://github.com/django/django/compare/7f2485b4d180...17c66e6fe77b
-- Aymeric. On 22 déc. 2013, at 12:14, Aymeric Augustin <aymeric.augus...@polytechnique.org> wrote: > I’ve updated the pull request with these changes as well as a few minor > comments made by reviewers. > > https://github.com/django/django/pull/2089 > > I think it’s worth merging in that state. I still have a list of about 20 > things to check, but none of them is likely to change anything fundamental. > > -- > Aymeric. > > > > On 21 déc. 2013, at 17:01, Aymeric Augustin > <aymeric.augus...@polytechnique.org> wrote: > >> Hello, >> >> Based on the feedback I received through several channels (GitHub, IRC, >> private email) I’m planning to make two API changes before merging this pull >> request. >> >> >> (1) Remove auto-discovery of AppConfig in application modules >> >> I implemented this shim to make it possible to take advantage of app configs >> without changing the format of INSTALLED_APPS. I wanted to increase >> backwards-compatibility and accelerate adoption in pluggable apps. I also >> wanted to keep INSTALLED_APPS short and readable in the common case and >> avoid this pattern: >> >> INSTALLED_APPS = ( >> ‘django.contrib.admin.app.AdminConfig', >> 'django.contrib.auth.app.AuthConfig', >> 'django.contrib.contenttypes.app.ContentTypesConfig', >> 'django.contrib.sessions.app.SessionsConfig', >> 'django.contrib.messages.app.MessagesConfig', >> 'django.contrib.staticfiles.app.StaticFilesConfig’, >> # et caetera ad nauseam >> # cold enterprisey thorny feelings >> ) >> >> Astute readers will note that MIDDLEWARE_CLASSES looks even worse but I’m >> not buying the “make it consistently ugly" argument :-) >> >> (Also I haven’t prepared any changes to Django’s contrib apps yet.) >> >> However, I can also list a few reasons not to provide this shim: >> >> - “There should be one-- and preferably only one --obvious way to do it.” >> - “Explicit is better than implicit.” >> - Django shouldn't encourage writing code in __init__.py files because it’s >> a common cause of import loops eg. when a submodule attempts to import an >> object defined in a parent package. Strictly speaking, importing the base >> AppConfig class and implementing a subclass is safe: it doesn’t even load >> the Django settings. But it’s still a code smell. >> - Python ≥ 3.3 introduces support for namespace packages (PEP 420) by making >> __init__.py files optional. I’m pretty sure someone will find a good reason >> to implement a Django app as a namespace package; then they couldn’t take >> advantage of this convention any more. Besides, skipping __init__.py files >> entirely might become a good practice in the next years. >> >> One could also say that the explicit version makes it obvious which >> applications use an application configuration and which don’t, but I don’t >> find the difference between “using the application’s default AppConfig” and >> “using Django’s default AppConfig” relevant. >> >> I’m ambivalent about this question. Considering that it can easily be added >> later, I’m leaning towards not including it for now. The reverse would be >> much more complicated. >> >> >> (2) Move the code back into django.apps >> >> I originally wrote all the code in django.apps. >> >> Then I realized it could cause confusion because “apps” shipped with Django >> are in django.contrib, not in django.apps, and I moved the code to >> django.contrib.apps. One could argue that apps are a core concept. >> >> However, there are two strong arguments for moving the code back to its >> original location. >> >> - Most APIs intended to be imported by user code live outside of core: >> “django.forms”, “django.db.models”, “django.templates”, “django.views”, etc. >> - Most of the code in django.core would better live in a shallower >> hierarchy. For instance “django.core.urlresolvers” doesn’t strike me a >> superior to “django.urls”. Besides “flat is better than nested”. >> >> >> Let me if you have additional arguments in favor of or against this changes. >> I’ll try to implement them tomorrow or on Monday. >> >> -- >> Aymeric. >> >> >> >> On 20 déc. 2013, at 16:37, Aymeric Augustin >> <aymeric.augus...@polytechnique.org> wrote: >> >>> Merge request >>> >>> I sent a pull request implementing my second goal: >>> https://github.com/django/django/pull/2089. It allows customizing >>> application names in the admin. >>> >>> A handful of core developers were kind enough to oversee my efforts. Their >>> feedback on the design has been positive. However, as far as I know, that >>> part of the branch hasn’t received a full review yet. It would be helpful >>> if someone had the time to read through the commits. >>> >>> Custom app names are implemented in a single commit that changes four lines >>> and doesn’t have tests and docs yet. It doesn’t even deal with breadcrumbs. >>> I’ve included it as a proof-of-concept but I can leave it aside for now if >>> you find that too cavalier. >>> >>> What I reallly want to merge is the seventeen previous previous commits. >>> Their sole purpose is to make the app cache the only piece of code that >>> knows about the INSTALLED_APPS settings. That’s a prerequisite for changing >>> its format and support custom application configurations. >>> >>> It’s a fairly large refactoring that, as far as I know, should be backwards >>> compatible. Of course, third-party apps that use INSTALLED_APPS will need >>> to be updated to support the new format. But as long as you aren’t using >>> custom application configurations, everything should still work as usual. >>> Since I’ve rewritten the app cache population code, the import sequence >>> could be different, and that could create import loops in some projects. >>> Unfortunately here’s no way to tell for sure besides asking people to try >>> it. It could just as well suppress import loops in other projects, except >>> there aren’t many active projects with import loops :-) >>> >>> Again, if you see a good reason not to merge it, please tell me as soon as >>> possible! >>> >>> The API for customizing an application’s configuration comes in two >>> flavors. Currently the only attribute you can customize is “verbose_name”. >>> >>> 1) You’re the author of a pluggable app called Rock ’n’ roll and you’re fed >>> up with seeing Rock_N_Roll in the admin: >>> >>> # rock_n_roll/__init__.py >>> >>> from django.core.apps import base >>> >>> class AppConfig(base.AppConfig): >>> verbose_name = "Rock ’n’ roll" >>> >>> The class has to be called “AppConfig” for Django to discover it. >>> >>> 2) You’re an user of Rock ’n’ roll and you wish it had a more palatable >>> name: >>> >>> # myproject/apps.py >>> >>> from django.core.apps import base >>> >>> class GypsyJazzConfig(base.AppConfig): >>> name = "rock_n_roll" >>> verbose_name = "Gypsy jazz" >>> >>> # myproject/settings.py >>> >>> INSTALLED_APPS = [ >>> # … >>> 'myproject.apps.GypsyJazzConfig', >>> # … >>> ] >>> >>> The class can live anywhere, but it must provide a “name” so that Django >>> knows what application it relates to. >>> >>> Notes for reviewers >>> >>> There’s one part of the patch I don’t like much: the AppCache methods >>> listed under ### DANGEROUS METHODS ###. I’ve made it very obvious that >>> they’re private APIs and they’re only used in tests. At this time I don’t >>> have any better ideas. It seems reasonable to decide that we’ll improve it >>> later them if the need arises. >>> >>> Day 5 >>> >>> I thought about the API. The end result is described above. I’m skipping >>> the details as I’m afraid they would add more confusion than clarity. >>> >>> Day 6 >>> >>> I began laying the ground for AppConfig classes in INSTALLED_APPS by >>> searching a way to remove all direct references to INSTALLED_APPS to use >>> app_cache.get_app_configs() instead. >>> >>> Note that app_cache.get_app_configs() calls app_cache.populate(). Therefore >>> iterating on app_cache.get_app_configs() will trigger some imports, while >>> iterating on settings.INSTALLED_APPS didn’t. This may result in different >>> behavior, including import loops, but I’m afraid there’s no way around >>> this. Hopefully the end result will be more deterministic than what we >>> currently have, as Django will populate the app cache earlier. >>> >>> I don’t know how I’m going to deal with >>> override_settings(INSTALLED_APPS=[…]). There’s a few dozen of these in the >>> test suite. Until now I’ve refrained from implementing AppCache.reload() >>> because it means >>> https://code.djangoproject.com/attachment/ticket/3591/app-loading.2.diff#L165 >>> and that scares me. I might end up reusing AppCache.set_available_apps(), >>> although currently it only supports restricting the set of installed apps. >>> >>> Day 7 >>> >>> My plan to remove direct references to INSTALLED_APPS with >>> get_app_configs() failed miserably. get_app_configs() calls populate() >>> which imports every application. This triggers import loops. I also spent >>> some time implementing support for override_settings(INSTALLED_APPS=…) at >>> the level of the app cache but that got messy very quickly. >>> >>> In fact, we need to be able to iterate on applications to discover things >>> (like translations and templates) without importing models. Currently the >>> app cache’s API doesn’t provide that feature. My next idea is to split >>> populate() in two stages: >>> - First stage does everything that doesn’t require importing models >>> modules. Application modules aren’t supposed to import much, if anything. >>> - Second stage imports models modules and adds them to the app_config >>> objects. >>> >>> This design makes it possible to replace `for app in INSTALLED_APPS` with a >>> method of the app cache without triggering an import cascade. This is >>> especially important when it occurs at the module level (which is awful but >>> we have such code in django/template/loaders/app_directories.py). >>> >>> In the mean time, I deborgified the app cache, which went surprisingly >>> well, and renormalized the “installed” flag. I pushed these changes to >>> master. >>> >>> Day 8 >>> >>> I finally broke through the wall of circular imports I kept running into as >>> soon as I touched populate() or attempted to use the app cache during the >>> import sequence. I replaced populate() with the two-stage population >>> process. >>> >>> This had some interesting side effects on the implementation of the app >>> cache, specifically on the registration of models. In my opinion it has >>> become more resilient and straightforward. For instance I find that >>> unconditionally calling an idempotent method is more robust than >>> conditionally calling it depending on some global state that’s mutated >>> throughout a stack of recursive calls. >>> >>> I also created private APIs to mess with the list of registered apps, >>> because Django has many tests doing just that, and it’s better to do it >>> through an API. >>> >>> Day 9 >>> >>> I replaced all direct uses of INSTALLED_APPS in django/ and tests/ with >>> methods of the app cache. It was tedious and touched lots of dubious tests. >>> >>> Day 10 >>> >>> I implemented support for custom AppConfig subclasses in INSTALLED_APPS, >>> and I wrote a proof-of-concept in the admin. >>> >>> -- >>> Aymeric. >>> >> > -- 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-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@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/741442BC-1A41-4299-A75A-1F7CAA13215A%40polytechnique.org. For more options, visit https://groups.google.com/groups/opt_out.
