#31180 closed New feature (fixed)
Deprecate default_app_config.
| Reported by: | Aymeric Augustin | Owned by: | Aymeric Augustin |
|---|---|---|---|
| Component: | Core (Other) | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Ready for checkin | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
Many pluggable applications are configured with a dotted path to the app (e.g. "django.contrib.admin") in INSTALLED_APPS.
The application then relies on default_app_config for locating its AppConfig class (e.g. "django.contrib.admin.apps.AdminConfig").
default_app_config was intended to allow author of pluggable applications to take advantage of AppConfig features without require their users to change INSTALLED_APPS, in order to increase adoption.
Unfortunately, that reduced the incentive for directly putting paths to AppConfig in INSTALLED_APPS. Even though that was the official recommendation, it didn't gain traction.
If I had better anticipated this result, I wouldn't have introduced default_app_config in the app-loading refactor in Django 1.7.
We've spent about five years failing at getting people to adopt explicit configuration. It's time to admit failure. Practicality beats purity.
With the benefit of hindsight, I'm proposing to load AppConfig classes from a conventional location, namely the apps submodule inside an application.
This is inconsistent with Django's design philosophy, as Django favors configuration over convention, but not much worse than the default_app_config convention.
There's a small risk of backwards-incompatibility. Django could import an apps.py submodule designed for another purpose and that could have side effects. I think that risk is low.
Change History (11)
comment:1 by , 6 years ago
| Component: | Core (Serialization) → Core (Other) |
|---|---|
| Summary: | Deprecate default_app_config → Deprecate default_app_config. |
| Triage Stage: | Unreviewed → Accepted |
comment:3 by , 6 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:5 by , 5 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
Needs a final squash, but looks good.
Pushed some edits so will just give Aymeric a chance to comment again.
comment:6 by , 5 years ago
| Patch needs improvement: | set |
|---|---|
| Triage Stage: | Ready for checkin → Accepted |
This isn't quite ready.
There's a test failure from the new check, for a test added in dd1ca50b096bf0351819aabc862e91a9797ddaca (after the PR here was first created):
======================================================================
ERROR: test_translation_loading (i18n.tests.TranslationLoadingTests)
"loading_app" does not have translations for all languages provided by
----------------------------------------------------------------------
Traceback (most recent call last):
...
File "django/django/apps/registry.py", line 354, in set_installed_apps
self.populate(installed)
File "django/django/apps/registry.py", line 91, in populate
app_config = AppConfig.create(entry)
File "django/django/apps/config.py", line 235, in create
app_name, app_config_class.__module__, app_config_class.__name__,
django.core.exceptions.ImproperlyConfigured: Cannot import 'loading_app'. Check that 'i18n.loading_app.apps.LoadingAppConfig.name' is correct.
I suspect that's just an artifact of the test case.
Plus, there are a couple of new review comments to double-check.
comment:7 by , 5 years ago
| Patch needs improvement: | unset |
|---|
I believe all feedback was addressed and the PR just needs squashing and merging.
comment:8 by , 5 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|