Re: App-loading reloaded - custom app names in the admin

[Aymeric Augustin]

After the merge, some Selenium tests (which I don’t run locally in general) 
failed on Jenkins. That’s a test isolation issue: an AppDirectoriesFinder 
instance kept a cache of an incomplete list of applications, preventing later 
tests from finding static files correctly.

I wanted to review the methods that add and remove apps to the app cache anyway 
— that’s the “part of the patch I don’t like much” I was talking about in my 
first email. I’ve sent a new pull request for this purpose: 
https://github.com/django/django/pull/2103.

It introduces a new API called override_list_settings to alter settings that 
are lists of values. It allows simply using override_settings(INSTALLED_APPS=…) 
or override_list_settings(INSTALLED_APPS=…). django.test now contains some 
references to django.apps but that seems reasonable to be as the app cache is a 
core concept of Django. Overall the result seems much better, both in terms of 
API and implementation.

I’ll merge that once I get a +1 from another core dev.

-- 
Aymeric.



On 22 déc. 2013, at 17:10, Aymeric Augustin 
 wrote:

> Merged: https://github.com/django/django/compare/7f2485b4d180...17c66e6fe77b
> 
> -- 
> Aymeric.
> 
> 
> 
> On 22 déc. 2013, at 12:14, Aymeric Augustin 
>  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 
>>  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”, 

Re: App-loading reloaded - custom app names in the admin

[Aymeric Augustin]

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 
 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 
>  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 

Re: App-loading reloaded - custom app names in the admin

[Aymeric Augustin]

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 
 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 

App-loading reloaded - custom app names in the admin

[Aymeric Augustin]

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