Make it possible to force the use of a given AppConfig subclass. #2192

Closed
wants to merge 17 commits into
from

Conversation

Projects
None yet
5 participants
@mjtamlyn
Member

mjtamlyn commented Jan 21, 2014

No description provided.

Make it possible to force the use of a given AppConfig subclass.
This behaviour is instantly deprecated, it's a migration aid not a new
feature.
@freakboy3742

This comment has been minimized.

Show comment Hide comment
@freakboy3742

freakboy3742 Jan 21, 2014

I haven't downloaded and run the code, but from inspection, looks good to me.

I haven't downloaded and run the code, but from inspection, looks good to me.

This comment has been minimized.

Show comment Hide comment
@freakboy3742

freakboy3742 Jan 21, 2014

Hang on - I take that back - have we lost the behaviour where an app that isn't installed via an explicit AppConfig, and doesn't have a default_app_config, is installed with a default AppConfig? (essentially, the last "else" statement in the old version) Raising this because this else case should probably raise a PendingDeprecationWarning as well, since we want to discourage non-AppConfig approaches.

Hang on - I take that back - have we lost the behaviour where an app that isn't installed via an explicit AppConfig, and doesn't have a default_app_config, is installed with a default AppConfig? (essentially, the last "else" statement in the old version) Raising this because this else case should probably raise a PendingDeprecationWarning as well, since we want to discourage non-AppConfig approaches.

This comment has been minimized.

Show comment Hide comment
@mjtamlyn

mjtamlyn Jan 21, 2014

Owner

No, it's lines 88-89 in the new version. I don't want to deprecate this - I think it's fine for apps which don't need any AppConfig related functionality to not provide an AppConfig. For example an "admin template overrides" app which doesn't need a ready() method. I don't think this should be deprecated - I don't believe that every single app should require an AppConfig, just every single app which has a use for them.

Owner

mjtamlyn replied Jan 21, 2014

No, it's lines 88-89 in the new version. I don't want to deprecate this - I think it's fine for apps which don't need any AppConfig related functionality to not provide an AppConfig. For example an "admin template overrides" app which doesn't need a ready() method. I don't think this should be deprecated - I don't believe that every single app should require an AppConfig, just every single app which has a use for them.

mjtamlyn added some commits Jan 21, 2014

Give the admin a default_app_config.
Also introduce a PlainAdminConfig which does not call autodiscover()
Fixes #21831 -- Give auth a default_app_config.
This installs the checks on the user model. By putting this in an
AppConfig, it ensures that this does not get registered as a side effect
of importing auth.
@mjtamlyn

This comment has been minimized.

Show comment Hide comment
@mjtamlyn

mjtamlyn Jan 21, 2014

Member

(first commit fixes https://code.djangoproject.com/ticket/21829 - I'll amend the commit message if this gets merged, but don't want to remove @freakboy3742's comments)

Member

mjtamlyn commented Jan 21, 2014

(first commit fixes https://code.djangoproject.com/ticket/21829 - I'll amend the commit message if this gets merged, but don't want to remove @freakboy3742's comments)

- # HACK.
- if module_label not in installed_app_names:
+ if module_label in installed_app_names:
+ continue

This comment has been minimized.

Show comment Hide comment
@mjtamlyn

mjtamlyn Jan 21, 2014

Member

I'm a little unsure about this change, but without it we end up with django.contrib.admin and django.contrib.admin.apps.PlainAdminConfig in the INSTALLED_APPS

@mjtamlyn

mjtamlyn Jan 21, 2014

Member

I'm a little unsure about this change, but without it we end up with django.contrib.admin and django.contrib.admin.apps.PlainAdminConfig in the INSTALLED_APPS

This comment has been minimized.

Show comment Hide comment
@carljm

carljm Jan 21, 2014

Member

I'm not sure I understand how this change prevents that...

@carljm

carljm Jan 21, 2014

Member

I'm not sure I understand how this change prevents that...

This comment has been minimized.

Show comment Hide comment
@mjtamlyn

mjtamlyn Jan 21, 2014

Member

installed_app_names contains the names of the apps, which is not necessarily what is in INSTALLED_APPS. Because we now have django.contrib.admin.apps.PlainAdminConfig, and we look at contrib for places to find tests and pick up django.contrib.admin this resulted in trying to add it twice, and thus (in the old version) raising a warning. Whether this is still a problem is an interesting question, but it seemed wrong to me to be re-adding django.contrib.admin with a default AppConfig. As @aaugustin knows more about this I'd like him to look at the change

@mjtamlyn

mjtamlyn Jan 21, 2014

Member

installed_app_names contains the names of the apps, which is not necessarily what is in INSTALLED_APPS. Because we now have django.contrib.admin.apps.PlainAdminConfig, and we look at contrib for places to find tests and pick up django.contrib.admin this resulted in trying to add it twice, and thus (in the old version) raising a warning. Whether this is still a problem is an interesting question, but it seemed wrong to me to be re-adding django.contrib.admin with a default AppConfig. As @aaugustin knows more about this I'd like him to look at the change

This comment has been minimized.

Show comment Hide comment
@carljm

carljm Jan 21, 2014

Member

Ok, I see. I was misunderstanding the implementation because I didn't notice that there was code after the 'else' that the 'continue' skipped.

@carljm

carljm Jan 21, 2014

Member

Ok, I see. I was misunderstanding the implementation because I didn't notice that there was code after the 'else' that the 'continue' skipped.

This comment has been minimized.

Show comment Hide comment
@shaib

shaib Jan 22, 2014

Member

Actually, I find this a little misleading too. Why are lines 178--181 out of the else block?

@shaib

shaib Jan 22, 2014

Member

Actually, I find this a little misleading too. Why are lines 178--181 out of the else block?

@mjtamlyn

This comment has been minimized.

Show comment Hide comment
@mjtamlyn

mjtamlyn Jan 21, 2014

Member

I think we should only use default_app_config for the apps where we have a requirement for the AppConfig.ready(). Where it's a "nice to have" like in contrib.sites and just provides translation support for the admin, we do not need to force it.

This does however bring up an interesting question: if something else in contrib (or $THIRDPARTYAPP) wishes to convert to an AppConfig and need this "forced default" behaviour after Django 1.9 then we are potentially expecting them to push a breaking change as the app would now raise ImproperlyConfigured or similar when installed without the relevant AppConfig. Now personally I think this is ok, at least for third party, but it's worth mentioning.

So perhaps it's better to use default_app_config everywhere now?

Alternatively, we need to extend the idea of default_app_config to allow a by-app deprecation of installing it without the AppConfig. This does however mean that some third party apps may continue to rely on it for ever and not progress their deprecation, which would be bad. So the current plan is probably best IMO.

Member

mjtamlyn commented Jan 21, 2014

I think we should only use default_app_config for the apps where we have a requirement for the AppConfig.ready(). Where it's a "nice to have" like in contrib.sites and just provides translation support for the admin, we do not need to force it.

This does however bring up an interesting question: if something else in contrib (or $THIRDPARTYAPP) wishes to convert to an AppConfig and need this "forced default" behaviour after Django 1.9 then we are potentially expecting them to push a breaking change as the app would now raise ImproperlyConfigured or similar when installed without the relevant AppConfig. Now personally I think this is ok, at least for third party, but it's worth mentioning.

So perhaps it's better to use default_app_config everywhere now?

Alternatively, we need to extend the idea of default_app_config to allow a by-app deprecation of installing it without the AppConfig. This does however mean that some third party apps may continue to rely on it for ever and not progress their deprecation, which would be bad. So the current plan is probably best IMO.

@carljm

This comment has been minimized.

Show comment Hide comment
@carljm

carljm Jan 21, 2014

Member

As I said on the mailing list, this looks fine to me except that I think we should wait on the deprecation until we have some actual experience with the feature, rather than backing ourselves into a corner before we've even seen how the feature is used.

If we are explicitly allowing third-party apps that don't need AppConfig to not have one, indefinitely, then I think that is an even stronger argument why default_app_config should remain a permanent feature. Because an app may want to make that transition (from "no AppConfig" to "with AppConfig") anytime in the future, including apps that haven't even been written yet. So if we allow "no AppConfig" but remove default_app_config, we are saying in perpetuity that once you have released an app without an AppConfig, the only way you can start relying on one is by pushing a backwards-incompatible change on your users. To me that sounds like a great way to discourage use of AppConfig and make things unnecessarily difficult for app authors.

Member

carljm commented Jan 21, 2014

As I said on the mailing list, this looks fine to me except that I think we should wait on the deprecation until we have some actual experience with the feature, rather than backing ourselves into a corner before we've even seen how the feature is used.

If we are explicitly allowing third-party apps that don't need AppConfig to not have one, indefinitely, then I think that is an even stronger argument why default_app_config should remain a permanent feature. Because an app may want to make that transition (from "no AppConfig" to "with AppConfig") anytime in the future, including apps that haven't even been written yet. So if we allow "no AppConfig" but remove default_app_config, we are saying in perpetuity that once you have released an app without an AppConfig, the only way you can start relying on one is by pushing a backwards-incompatible change on your users. To me that sounds like a great way to discourage use of AppConfig and make things unnecessarily difficult for app authors.

mjtamlyn added some commits Jan 21, 2014

Make CheckRegistry.register() idempotent.
I can't think of a good reason why it should be a problem if a check is
registered twice, but we are not going to want it to run twice and print
the errors twice.
Make AdminConfig the default.
This is backwards incompatible if you do not normally use autodiscovery
and autodiscovery being called causes you errors. As far as I can tell,
if you do use autodiscovery autodiscover() being called twice does not
result in errors.
@mjtamlyn

This comment has been minimized.

Show comment Hide comment
@mjtamlyn

mjtamlyn Jan 21, 2014

Member

Ok, I've un-deprecated this feature and documented it. I have however recommended that explicit is better, though I have some personal reservations about that. For now, I've not added default_app_config to all contrib apps, which I think I probably should do.

I have also:

  • Made autodiscover-based AdminConfig the default, but provided a subclass which does not do this
  • Tweaked the checks framework to be idempotent (when INSTALLED_APPS kept changing in the test suite the checks kept getting added to)
Member

mjtamlyn commented Jan 21, 2014

Ok, I've un-deprecated this feature and documented it. I have however recommended that explicit is better, though I have some personal reservations about that. For now, I've not added default_app_config to all contrib apps, which I think I probably should do.

I have also:

  • Made autodiscover-based AdminConfig the default, but provided a subclass which does not do this
  • Tweaked the checks framework to be idempotent (when INSTALLED_APPS kept changing in the test suite the checks kept getting added to)
django/contrib/admin/apps.py
+ checks.register('admin')(check_admin_app)
+
+
+class PlainAdminConfig(AdminConfig):

This comment has been minimized.

Show comment Hide comment
@carljm

carljm Jan 21, 2014

Member

This is fine, and I suppose it makes some sense to have the default admin app config be the base class - but in general I would go the other way and have the one that does less be the base class and the discovery one subclass and extend it. (Liskov something something mumble.)

@carljm

carljm Jan 21, 2014

Member

This is fine, and I suppose it makes some sense to have the default admin app config be the base class - but in general I would go the other way and have the one that does less be the base class and the discovery one subclass and extend it. (Liskov something something mumble.)

@@ -29,7 +29,8 @@ def my_check(apps, **kwargs):
def inner(check):
check.tags = tags
- self.registered_checks.append(check)
+ if check not in self.registered_checks:

This comment has been minimized.

Show comment Hide comment
@carljm

carljm Jan 21, 2014

Member

Under what conditions is this needed?

@carljm

carljm Jan 21, 2014

Member

Under what conditions is this needed?

This comment has been minimized.

Show comment Hide comment
@mjtamlyn

mjtamlyn Jan 21, 2014

Member

The specific situation I introduce it for is when INSTALLED_APPS is hacked around with so ready() gets called multiple times (whenever an app is added back in). In general though, if for some reason I accidentally register the same function twice, we shouldn't print the errors twice.

@mjtamlyn

mjtamlyn Jan 21, 2014

Member

The specific situation I introduce it for is when INSTALLED_APPS is hacked around with so ready() gets called multiple times (whenever an app is added back in). In general though, if for some reason I accidentally register the same function twice, we shouldn't print the errors twice.

This comment has been minimized.

Show comment Hide comment
@carljm

carljm Jan 21, 2014

Member

Ah makes sense.

@carljm

carljm Jan 21, 2014

Member

Ah makes sense.

docs/ref/applications.txt
+.. note::
+
+ It is recommended to tell users to add the full path to the ``AppConfig``
+ subclass. However, it is possible to force a particular default

This comment has been minimized.

Show comment Hide comment
@carljm

carljm Jan 21, 2014

Member

I think 'force' is misleading language here. It doesn't force anything, it's a fallback default. If a user wants to use a different AppConfig they can still refer to it explicitly in INSTALLED_APPS.

@carljm

carljm Jan 21, 2014

Member

I think 'force' is misleading language here. It doesn't force anything, it's a fallback default. If a user wants to use a different AppConfig they can still refer to it explicitly in INSTALLED_APPS.

docs/ref/applications.txt
@@ -78,6 +78,14 @@ to determine which application this configuration applies to. You can define
any attributes documented in the :class:`~django.apps.AppConfig` API
reference.
+.. note::
+
+ It is recommended to tell users to add the full path to the ``AppConfig``

This comment has been minimized.

Show comment Hide comment
@carljm

carljm Jan 21, 2014

Member

Personally I would not say this is recommended; I think using default_app_config is a fine option, and users can be explicit only if they need a nonstandard AppConfig. I think we should at least be consistent - if we recommend long-form as preferable, we should use it throughout docs/examples/project-template etc; if we're not doing it ourselves, no point in claiming that we recommend it.

@carljm

carljm Jan 21, 2014

Member

Personally I would not say this is recommended; I think using default_app_config is a fine option, and users can be explicit only if they need a nonstandard AppConfig. I think we should at least be consistent - if we recommend long-form as preferable, we should use it throughout docs/examples/project-template etc; if we're not doing it ourselves, no point in claiming that we recommend it.

This comment has been minimized.

Show comment Hide comment
@mjtamlyn

mjtamlyn Jan 21, 2014

Member

I have the same conflict of feeling.

@mjtamlyn

mjtamlyn Jan 21, 2014

Member

I have the same conflict of feeling.

docs/ref/contrib/gis/tutorial.txt
@@ -116,7 +116,7 @@ and ``world`` (your newly created application)::
INSTALLED_APPS = (
'django.contrib.admin.apps.AdminConfig',
- 'django.contrib.auth',
+ 'django.contrib.auth.apps.AuthConfig',

This comment has been minimized.

Show comment Hide comment
@carljm

carljm Jan 21, 2014

Member

This change seems to be going in the opposite direction from the rest of the PR (i.e. you removed a bunch of long-form usage for admin, so why are we changing this to long-form usage for auth)?

@carljm

carljm Jan 21, 2014

Member

This change seems to be going in the opposite direction from the rest of the PR (i.e. you removed a bunch of long-form usage for admin, so why are we changing this to long-form usage for auth)?

This comment has been minimized.

Show comment Hide comment
@mjtamlyn

mjtamlyn Jan 21, 2014

Member

Well spotted.

@mjtamlyn

mjtamlyn Jan 21, 2014

Member

Well spotted.

docs/ref/applications.txt
@@ -78,6 +78,12 @@ to determine which application this configuration applies to. You can define
any attributes documented in the :class:`~django.apps.AppConfig` API
reference.
+.. note::
+
+ It is possible to ensure a particular default ``AppConfig`` subclass by

This comment has been minimized.

Show comment Hide comment
@carljm

carljm Jan 21, 2014

Member

I think the wording of this note is a bit unclear, as it doesn't clarify which __init__.py, and doesn't really explain what it means to "ensure a particular default ...". I think it would be clearer if default_app_config were mentioned further up, right after the code block, in the main flow of the text rather than in a note. Something like:

You can make this AppConfig subclass the default for your app by setting
``default_app_config = "rock_n_roll.apps.RockNRollConfig"`` in ``rocknroll/__init__.py`` --
that will cause this ``AppConfig`` to be used if a user has just ``"rocknroll"`` in their 
INSTALLED_APPS. Alternatively, you can tell your users to put
``"rocknroll.apps.RockNRollConfig"`` in their INSTALLED_APPS.
@carljm

carljm Jan 21, 2014

Member

I think the wording of this note is a bit unclear, as it doesn't clarify which __init__.py, and doesn't really explain what it means to "ensure a particular default ...". I think it would be clearer if default_app_config were mentioned further up, right after the code block, in the main flow of the text rather than in a note. Something like:

You can make this AppConfig subclass the default for your app by setting
``default_app_config = "rock_n_roll.apps.RockNRollConfig"`` in ``rocknroll/__init__.py`` --
that will cause this ``AppConfig`` to be used if a user has just ``"rocknroll"`` in their 
INSTALLED_APPS. Alternatively, you can tell your users to put
``"rocknroll.apps.RockNRollConfig"`` in their INSTALLED_APPS.
from django.utils.translation import ugettext_lazy as _
-class AdminConfig(AppConfig):
+class PlainAdminConfig(AppConfig):
+ """Simple AppConfig which does not do automatic discovery."""

This comment has been minimized.

Show comment Hide comment
@freakboy3742

freakboy3742 Jan 21, 2014

Member

100% bike shedding, but given the docstring, SimpleAdminConfig strikes me as a better name here.

@freakboy3742

freakboy3742 Jan 21, 2014

Member

100% bike shedding, but given the docstring, SimpleAdminConfig strikes me as a better name here.

@freakboy3742

This comment has been minimized.

Show comment Hide comment
@freakboy3742

freakboy3742 Jan 21, 2014

Member

Updated review now that all the ready() code is in place -- the code looks good for what it's trying to do. My only concern is the amount to which it rolls back the "explicit AppConfig" angle in INSTALLED_APPS. Personally, I think I can live with the patch as is, but I think our community discussion is getting a bit fractured and I'm not convinced we're still all on the same page (or, at least, some seem to be a couple of pages behind).

Member

freakboy3742 commented Jan 21, 2014

Updated review now that all the ready() code is in place -- the code looks good for what it's trying to do. My only concern is the amount to which it rolls back the "explicit AppConfig" angle in INSTALLED_APPS. Personally, I think I can live with the patch as is, but I think our community discussion is getting a bit fractured and I'm not convinced we're still all on the same page (or, at least, some seem to be a couple of pages behind).

django/apps/base.py
- # app config class.
- if not mod_path:
- raise
+ if module is not None:

This comment has been minimized.

Show comment Hide comment
@shaib

shaib Jan 22, 2014

Member

Style nitpick: use "else:" here instead of the If. Then you also don't need to erase module in the except.

@shaib

shaib Jan 22, 2014

Member

Style nitpick: use "else:" here instead of the If. Then you also don't need to erase module in the except.

django/apps/base.py
+
+ # Raise the original exception when entry cannot be a path to an
+ # app config class.
+ if not mod_path:

This comment has been minimized.

Show comment Hide comment
@shaib

shaib Jan 22, 2014

Member

if not (mod_path and cls_name) for the case where entry ends with a ".".

@shaib

shaib Jan 22, 2014

Member

if not (mod_path and cls_name) for the case where entry ends with a ".".

This comment has been minimized.

Show comment Hide comment
@mjtamlyn

mjtamlyn Jan 23, 2014

Member

I see your thinking, but when I tried this what actually happens in this case for now is it drops down to the following lines and you get (for path.to.:

ImportError: cannot import '' from 'path.to' which is actually pretty informative I think, as opposed to cannot import "path.to.".

@mjtamlyn

mjtamlyn Jan 23, 2014

Member

I see your thinking, but when I tried this what actually happens in this case for now is it drops down to the following lines and you get (for path.to.:

ImportError: cannot import '' from 'path.to' which is actually pretty informative I think, as opposed to cannot import "path.to.".

This comment has been minimized.

Show comment Hide comment
@shaib

shaib Jan 24, 2014

Member

Yes, I guess you're right.

@shaib

shaib Jan 24, 2014

Member

Yes, I guess you're right.

+ "cannot import name '%s' from '%s'" % (cls_name, mod_path))
+
+ # Check for obvious errors. (This check prevents duck typing, but
+ # it could be removed if it became a problem in practice.)

This comment has been minimized.

Show comment Hide comment
@shaib

shaib Jan 22, 2014

Member

Use ABC?

@shaib

shaib Jan 22, 2014

Member

Use ABC?

docs/releases/1.7.txt
- ``admin.autodiscover()`` from your URLconf.
+ :func:`~django.contrib.admin.autodiscover()`. You can consequently remove the
+ line from your ``urls.py``. If you do not wish to use the ``autodiscover()``
+ behaviour, simply, simply replace ``'django.contrib.admin'`` in

This comment has been minimized.

Show comment Hide comment
@shaib

shaib Jan 22, 2014

Member

simply use just one simply :)
also, behavior; I think Django docs standardize on US spelling.

@shaib

shaib Jan 22, 2014

Member

simply use just one simply :)
also, behavior; I think Django docs standardize on US spelling.

+ # Raise the original exception when entry cannot be a path to an
+ # app config class.
+ if not mod_path:
+ raise exception

This comment has been minimized.

Show comment Hide comment
@shaib

shaib Jan 24, 2014

Member

IIUC, writing default_app_config="" in your __init__.py will get this code to do raise None (there is no exception raised from importing the module, and yet mod_path is falsey).

@shaib

shaib Jan 24, 2014

Member

IIUC, writing default_app_config="" in your __init__.py will get this code to do raise None (there is no exception raised from importing the module, and yet mod_path is falsey).

This comment has been minimized.

Show comment Hide comment
@aaugustin

aaugustin Jan 24, 2014

Member

Reraising an exception like this doesn't work perfectly. See six.reraise.

@aaugustin

aaugustin Jan 24, 2014

Member

Reraising an exception like this doesn't work perfectly. See six.reraise.

@@ -171,8 +171,9 @@ def no_available_apps(self):
if module_found_in_labels:
if verbosity >= 2:
print("Importing application %s" % module_name)
- # HACK.

This comment has been minimized.

Show comment Hide comment
@aaugustin

aaugustin Jan 24, 2014

Member

Why does everyone keep removing those carefully crafted comments? ;-)

@aaugustin

aaugustin Jan 24, 2014

Member

Why does everyone keep removing those carefully crafted comments? ;-)

-The easiest solution is to put ``'django.contrib.admin.apps.AdminConfig'`` in
-your :setting:`INSTALLED_APPS` setting.
+By default, this happens automatically when you put
+``'django.contrib.admin'`` in your :setting:`INSTALLED_APPS` setting.

This comment has been minimized.

Show comment Hide comment
@aaugustin

aaugustin Jan 24, 2014

Member

This sentence contradicts the previous one.

@aaugustin

aaugustin Jan 24, 2014

Member

This sentence contradicts the previous one.

@aaugustin

This comment has been minimized.

Show comment Hide comment
@aaugustin

aaugustin Jan 24, 2014

Member

Updated patch: #2210

I still have to review all the comments here and to proof-read the docs.

Member

aaugustin commented Jan 24, 2014

Updated patch: #2210

I still have to review all the comments here and to proof-read the docs.

@aaugustin aaugustin closed this Jan 24, 2014

@mjtamlyn mjtamlyn deleted the mjtamlyn:default-app-config branch Jan 27, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment