Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixed #17304 -- Allow single-path and configured-path namespace packages as apps. #2212

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
2 participants
Owner

carljm commented Jan 25, 2014

Also document the conditions under which a namespace package may or may not be
a Django app, and raise a clearer error message in those cases where it may not
be.

@carljm carljm Fixed #17304 -- Allow single-path and configured-path namespace packa…
…ges as apps.

Also document the conditions under which a namespace package may or may not be
a Django app, and raise a clearer error message in those cases where it may not
be.
87db3a8

@aaugustin aaugustin commented on the diff Jan 25, 2014

django/apps/base.py
except AttributeError:
self.path = None
+ else:
+ # Convert paths to list because Python 3.3 _NamespacePath does
+ # not support indexing.
+ paths = list(paths)
+ if len(paths) > 1:
@aaugustin

aaugustin Jan 25, 2014

Owner

I think it's possible that len(paths) == 0, e.g. with a custom module loader. In that case we should set self.path = None.

(I know this used to crash with the previous implementation.)

@carljm

carljm Jan 25, 2014

Owner

Hmm, I started to work on this, and then I began to wonder about the wisdom of allowing AppConfig.path to ever be None. I think we may already have code in Django itself (for instance, the code I wrote for AppDirectoriesFinder yesterday) that would crash if any apps had a path of None, because it doesn't check if app_config.path before trying to call os.path.join on it. I wouldn't be surprised if there are other such cases, and I'd be very surprised if lots of third-party code doesn't end up being written that fails to account for this case.

As far as I know, in any situation where app_module.__path__ would be empty or not exist, that app would almost certainly not have worked pre-app-loading anyway, when we relied on app_module.__file__. So couldn't we instead just raise ImproperlyConfigured in case of an app where no path can be deduced from the module, and no path attribute is explicitly supplied on the AppConfig?

@carljm

carljm Jan 25, 2014

Owner

It seems to me that if we're documenting setting AppConfig.path explicitly, we are giving anyone hit by this problem a clear and explicit path forward that is more likely to solve their problem, and it's better to fail quickly and point them toward this solution than silently allow AppConfig.path to be None and wait for some poorly-written client code to break.

@aaugustin

aaugustin Jan 25, 2014

Owner

Yes, lots of things are likely to misbehave when path is None. However, I think Django supports loading apps from eggs -- we even have tests for that -- and in that case there's no meaningful path. Raising an exception in that case would be backwards-incompatible.

@carljm

carljm Jan 26, 2014

Owner

Well, zipped-egg packages actually have both __path__ and __file__, but both are bogus; not actual filesystem paths (they are "paths" that would be valid only if the egg were unzipped into a directory, even when it's not). So those types of apps would end up with an appconfig.path that isn't None, but isn't really usable either (though it probably doesn't break outright, since most uses of appconfig.path probably check os.path.exists or a variant.)

I'm actually not sure what types of packages either don't have __path__ or have an empty one. Do you know of any specific examples? Was there one that motivated you to add the except AttributeError clause? If they aren't cases that we explicitly supported in prior versions, ISTM it would be valid to say "if you want to use your funky module loader for a Django app, you need to give an explicit path on your AppConfig."

The tests that fail in Django's own test suite if I eliminate the except AttributeError clause are cases where something is passing in None as the app_module argument to AppConfig() -- but it seems to me that's a case that would probably be better if it failed right away? Unless it was intentional on your part that that should succeed?

The other case I'm aware of that would fail without that except check would be if someone passed in a module rather than a package (i.e. path.to.foo which is path/to/foo.py, not path/to/foo/__init__.py). I'm not aware of any good use case for that, but it seems that it "worked" in 1.6. Since it was never documented you could do that, I'm not sure we need to support it?

(Sorry if I'm revisiting things you already went over in depth in working on app-loading. I just feel like this release is kind of our chance to enforce a bit more sanity in this area; if we're going to do it, may as well do it all at once.)

@carljm

carljm Jan 26, 2014

Owner

(If you think it's worth exploring having AppConfig.path not allowed to be None, I would file a new ticket for that and merge this as-is, since clearly that would require more extensive work on the test suite. Otherwise I'll just go ahead with your suggestion to also make AppConfig.path None in the case where __path__ is empty.)

@carljm

carljm Jan 26, 2014

Owner

One further note and then I'll shut up and wait to hear your thoughts: if the file-module case I mentioned above (foo.py) is something we need to support, it would be easy to support with a fallback to os.path.dirname(app_module.__file__) if the module has no __path__.

So in summary, it seems possible to me that we could support everything we need to with the combo of supporting len-1 __path__ and __file__, and allow anything that doesn't have either of those to be an immediate error (which the user can of course get past with an explicit AppConfig.path).

But it's quite likely I'm wrong! You've spent a lot more time looking at this in the last month than I have.

@aaugustin

aaugustin Jan 26, 2014

Owner

I hadn't explored all these ideas. I just assumed that None would trigger exceptions and we'd fix them :-) Let's follow up on the ticket you created.

@aaugustin aaugustin commented on the diff Jan 25, 2014

docs/ref/applications.txt
@@ -188,6 +188,26 @@ Methods
def ready(self):
MyModel = self.get_model('MyModel')
+Namespace packages as apps (Python 3.3+)
+----------------------------------------
+
+Python versions 3.3 and later support Python packages without an
+``__init__.py`` file. These packages are known as "namespace packages" and may
+be spread across multiple directories at different locations on ``sys.path``
@aaugustin

aaugustin Jan 25, 2014

Owner

I think we usually write:

:envvar:`PYTHONPATH`
@carljm

carljm Jan 25, 2014

Owner

Hmm, those are not synonyms. The PYTHONPATH env var is just one (optional) way to add additional paths to sys.path; a PEP 420 namespace package could have multiple locations on sys.path with a completely empty PYTHONPATH.

It may be that we use PYTHONPATH as a synonym for sys.path elsewhere in the documentation, but if so, we are being imprecise and potentially confusing.

@aaugustin

aaugustin Jan 25, 2014

Owner

works for me :)

@aaugustin aaugustin commented on the diff Jan 25, 2014

tests/apps/tests.py
+
+
+@skipUnless(
+ sys.version_info > (3, 3, 0),
+ "Namespace packages sans __init__.py were added in Python 3.3")
+class NamespacePackageAppTests(TestCase):
+ # We need nsapp to be top-level so our multiple-paths tests can add another
+ # location for it (if its inside a normal package with an __init__.py that
+ # isn't possible). In order to avoid cluttering the already-full tests/ dir
+ # (which is on sys.path), we add these new entries to sys.path temporarily.
+ base_location = os.path.join(HERE, 'namespace_package_base')
+ other_location = os.path.join(HERE, 'namespace_package_other_base')
+ app_path = os.path.join(base_location, 'nsapp')
+
+ @contextmanager
+ def add_to_path(self, *paths):
@aaugustin

aaugustin Jan 25, 2014

Owner

We're adding things to sys.path in several tests.This function could live in django.test.utils and be reused by other tests.

@carljm

carljm Jan 25, 2014

Owner

That's a good point, but I think I'll pursue it in a separate follow-up commit. I find it clearer if the commit referencing a ticket confines itself to the changes relevant to that ticket, and refactorings are done separately.

Owner

aaugustin commented Jan 25, 2014

Great tests!

carljm added some commits Jan 25, 2014

@carljm carljm Merge branch 'master' into t17304
* master:
  Fixed a failing schema assertion.
  Fixed #21829 -- Added default AppConfigs.
  Fixed #21867 -- Removed AppStaticStorage; app paths are now AppConfig's job.
80caa70
@carljm carljm Minor wording tweaks in tests. 57f5712
@carljm carljm Merge branch 'master' into t17304
* master:
  Fixed #21873 -- Removed duplicate import.
  Fixed #21836 -- Improved transaction docs about autocommit mode
  Added ticket #14007 contributors to AUTHORS
c21c372
@carljm carljm Update the docs. 3554afa
Owner

carljm commented Jan 26, 2014

I moved AppConfig.path into the "configurable" section. I think what it says about zipped eggs having AppConfig.path = None is actually incorrect, as I mentioned in a different comment - zipped eggs will have an AppConfig.path that looks like a filesystem path but isn't a real one. So I think that note in the docs should be fixed regardless what approach we take with modules that are actually missing __path__.

Owner

carljm commented Jan 26, 2014

Since the one remaining issue under discussion here (about handling of empty __path__) is an issue already on master that isn't changed by this pull request, I decided to go ahead and merge this and continue discussion in a new ticket, #21874.

Squash-merged in 966b186.

@carljm carljm closed this Jan 26, 2014

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