Re: Round 2 for "A ModuleSpec Type for the Import System"

On Fri, Aug 9, 2013 at 6:58 PM, Eric Snow <[email protected]> wrote:
> A High-Level View
> -----------------
>
> ...

It would be really helpful if that high-level view were actually
included, as I'm having a lot of trouble wrapping my head around the
rest of the spec.  For that matter, some introductory examples to
contrast "before" and "after" for something that this changes would be
really nice at about this point.


> Path-based attributes:
>
> If any of these is set, it indicates that the module is path-based.  For
> reference, a path entry is a string for a location where the import
> system will look for modules, e.g. the path entries in ``sys.path`` or a
> package's ``__path__``).
>

What does "path-based" actually mean here?  On the one hand, you're
saying that a path entry is on sys.path or a __path__, but then we're
using an attribute called "filename".  Shouldn't it be called
path_entry or subpath, or location or something, if it's not required
to be a filename?  The overlap between path = sys.path and path =
filesystem path is way too confusing here.

> .. XXX Would a different name be better?  ``path_location``?

Yeah, definitely something other than filename.  ;-)

It might also help to explain that some modules can be loaded by
reference to a location, e.g. a filesystem path or a URL or something
of the sort -- having the location lets you load the module, but in
theory you could load that module under various names.  In contrast,
non-located modules can't be loaded in this fashion: modules created
by a meta path loader (such as builtins), or modules dynamically
created in code.  For these, the name is the only way to access them,
so they have an "origin" but not a "location".

Also, bear in mind that it's not just exotic locations like URLs that
aren't filenames.  zipimport uses pseudo-filenames that pretend a
zipfile is a directory, by prepending the zipfile's filename to a path
that's within the zipfile.  So, calling this "filename" is *really* a
bad idea; it's not always a filename for even stdlib importers, let
alone anything third-party!


> ``path``
>
> The list of path entries in which to search for submodules if this
> module is a package.  Otherwise it is ``None``.

This should probably be called submodule_path or
submodule_search_locations or something, to avoid even *more*
overloading of the word "path".  ;-)


> .. XXX add a path-based subclass?

Why?  What good would it do?


> ModuleSpec Methods
> ------------------
>
> ``from_loader(name, loader, *, is_package=None, origin=None, filename=None,
> cached=None, path=None)``
>
> .. XXX use a different name?

Seems fine to me: it's consistent w/other stdlib factory method names.


>    If not passed in, ``path`` is set to an empty list if
>    ``is_package`` is true.  Then the directory from ``filename`` is
>    appended to it, if possible.  If ``is_package`` is false, ``path``
>    stays unset.

How does this interact with namespace packages?  Does it?


> Sets the module's import-related attributes to the corresponding values
> in the module spec.  If a path-based attribute is not set on the spec,

Location-based?  ;-)

> ``load(module=None, *, is_reload=False)``
>
> This method captures the current functionality of and requirements on
> ``Loader.load_module()`` without any semantic changes, except one.
> Reloading a module when ``exec_module()`` is available actually uses
> ``module`` rather than ignoring it in favor of the one in
> ``sys.modules``, as ``Loader.load_module()`` does.

Interesting -- this could possibly be leveraged to implement
multi-version imports.

> ``module`` is only allowed when ``is_reload`` is true.

...or not.  ;)

> This means that
> ``is_reload`` could be dropped as a parameter.  However, doing so would
> mean we could not use ``None`` to indicate that the module should be
> pulled from ``sys.modules``.

Wait, what?  That doesn't seem true to me: why not just use the module
or pull one according to whether it's None or not?  What actual
difference does is_reload really make here?


> Regarding the first part of ``load()``, the following describes what
> happens.

I'm thinking maybe this should be parameterized to allow passing in a
'modules' dictionary other than sys.modules.  This would make
multi-version imports or other "isolated environment" imports more
viable, and factor out another global element of the import system.
That way, if you implement an isolated module system, you don't have
to duplicate or subclass ModuleSpec to perform the same loading
functionality.


> Unfortunately, the ability to proxy does not extend to ``id()``
> comparisons and ``isinstance()`` tests.

Who does id() tests on loaders?  isinstance() fudging, OTOH, is quite
doable.  See the ProxyTypes library on PyPI for an example; it's
2.x-only but I believe somebody has done a proof-of-concept port (due
to some __special__ methods being different or missing in 3.x)


> Finders
> -------
>
> Finders will now return ModuleSpec objects when ``find_module()`` is
> called rather than loaders.  For backward compatility, ``Modulespec``
> objects proxy the attributes of their ``loader`` attribute.

Has anybody looked at how this change affects pkgutil's (and
setuptools') generic function-based extensions to PEP 302?  Currently,
you can register specific loader types with these guys, but that'll
likely break if importlib is going to start wrapping loaders without
those tools' knowledge.

May I suggest adding a new finder method, find_module_spec() instead?
Then, implement it for finders that don't support it by calling
find_module() and wrapping the loader with a ModuleSpec.  This
approach would be less disruptive to code that already uses
find_module and inspects loader types to add extension protocols.


> Adding another similar method to avoid backward-compatibility issues
> is undersireable if avoidable.  The import APIs have suffered enough,
> especially considering ``PathEntryFinder.find_loader()`` was just
> added in Python 3.3.  The approach taken by this PEP should be
> sufficient to address backward-compatibility issues for
> ``find_module()``.

I'm not sure I'm following here: are you saying that all PEP 302
finders implemented by anyone, anywhere, must be changed *in order to
work at all*, when this lands in a *minor version change*?



> Other Changes
> -------------

This section doesn't address impact on pkgutil, which makes
significant use of the PEP 302 API.