Yet another app loading branch #2076
Conversation
| else: | ||
| raise | ||
|
|
||
| self.nesting_level -= 1 |
There was a problem hiding this comment.
Wouldn't that set the global nesting_level to -1 for apps without a models module?
There was a problem hiding this comment.
Indeed I understand that nesting_level is designed to handle reentrancy during the import sequence, but I don't know how it works exactly. I shall investigate.
There was a problem hiding this comment.
In fact that was easily fixed. See f2fd80a.
|
Ok, so I've run through this work one commit at a time and I didn't come up with any questions that weren't answered by future commits. I'm happy for this to be merged. The other primary goals (reliable init and verbose_name) seem to have fairly natural resolutions with the chosen design, which makes a lot of sense. There is still a significant open question as to how one goes about customising the AppConfig object for a given app name (i.e. in INSTALLED_APPS) but that's for another time. Overall, I'm happy to hit the merge button if no one else objects. |
| loads_installed=True, | ||
| loaded=False, | ||
| handled=set(), | ||
| postponed=[], |
There was a problem hiding this comment.
Does postponing an app more than once make sense? If not, this could be a set.
There was a problem hiding this comment.
I haven't spend much effort on populate(). This code is simply moved from django.db.models.loading.
Your comment makes sense but this isn't part of the scope of this pull request.
|
Hell, EDIT:// I think since 12146f3 |
This commit doesn't contain any code changes; it's purely a refactoring.
It was only storing redundant information. This is part of the effort to allow applications without a models module.
Its only difference with OrderedDict is that it didn't deepcopy its keys. However it wasn't used anywhere with models modules as keys, only as values. So this commit doesn't result in any change in functionality.
Since the original ones in django.db.models.loading were kept only for backwards compatibility, there's no need to recreate them. However, many internals of Django still relied on them. They were also imported in django.db.models. They never appear in the documentation, except a quick mention of get_models and get_app in the 1.2 release notes to document an edge case in GIS. I don't think that makes them a public API. This commit doesn't change the overall amount of global state but clarifies that it's tied to the app_cache object instead of hiding it behind half a dozen functions.
This is a step towards allowing applications without a models module.
get_app_errors() always returned an empty dictionary; this behavior is preserved in django.db.models.loading until that module is deprecated.
This commit is a refactoring with no change of functionality, according to the following invariants: - An app_label that was in app_configs and app_models stays in app_config and has its 'installed' attribute set to True. - An app_label that was in app_models but not in app_configs is added to app_configs and has its 'installed' attribute set to True. As a consequence, all the code that iterated on app_configs is modified to check for the 'installed' attribute. Code that iterated on app_models is rewritten in terms of app_configs. Many tests that stored and restored the state of the app cache were updated. In the long term, we should reconsider the usefulness of allowing importing models from non-installed applications. This doesn't sound particularly useful, can be a trap in some circumstances, and causes significant complexity in sensitive areas of Django.
Marginally improved creation of AppConfig stubs for non-installed apps.
Refactored get_app() to rely on that method. get_app() starts by calling _populate(), which goes through INSTALLED_APPS and, for each app, imports the app module and attempts to import the models module. At this point, no further imports are necessary to return the models module for a given app. Therefore, the implementation of get_app() can be simplified and the safeguards for race conditions can be removed. Besides, the emptyOK parameter isn't used anywhere in Django. It was introduced in d6c95e9 but not actually used nor documented, and it has just been carried around since then. Since it's an obscure private API, it's acceptable to stop supporting it without a deprecation path. This branch aims at providing first-class support for applications without a models module eventually. For backwards-compatibility, get_app() still raises ImproperlyConfigured when an app isn't found, even though LookupError is technically more correct. I haven't gone as far as to preserve the exact error messages. I've adjusted a few tests instead.
Refactored get_apps() to rely on that method. This commit is fully backwards-compatible.
It feels more natural for self.available_apps to contain app names (like INSTALLED_APPS) than app labels, and this is easy to implement now.
Since it's never called with more than one model at a time the current signature is needlessly complicated.
Several parts of Django call get_apps() with a comment along this lines of "this has the side effect of calling _populate()". I fail to see how this is better than just calling populate()!
Thanks Florian for catching this mistake.
Thanks Marc Tamlyn for the suggestion.
|
Rebased and merged. |
Currently (18 commits in) this branch allows creating applications without a models module while preserving full backwards compatibility