[Import-SIG] PEP 420 issue: extend_path

[Eric V. Smith]

On 05/08/2012 04:04 AM, Nick Coghlan wrote:

> What we're really talking about now is a wholesale replacement for
> find_module with the new semantics (i.e. allowing a string to be
> returned for namespace package paths), rather than PEP 382's extra
> processing step (where find_module would still be called, even if
> find_package_portion was defined).
>
> So, let's call the replacement finder method "find_loader", since
> that's really what it's for (find_module was always a bit of a
> misnomer, since the method returns a loader object, not a module).

Right. I'll call find_loader if it exists, else fall back to
find_module. Legacy finders will still work with PathFinder, they'll
just be unable to provide namespace package portions.

And just as important, modified finders will be able to be used with
legacy code that calls find_module.

> This is where I start to have sympathy for Antoine's point of view:
> we're overloading the meaning of returning a string from find_loader()
> to say two things:
> 1. Here's my sys.path entry
> 2. Please continue scanning sys.path for additional entries

That's correct on the two values to return. I'm just not very
sympathetic for a more complex signature when returning a string will
do, instead.

<discussion of callback API deleted>

>         if dir_found is None:
>             def dir_found(dirpath):
>                 msg = "Not importing directory {}: missing __init__"
>                 _warnings.warn(msg.format(dirpath), ImportWarning)

In this case (supporting the now-legacy find_module API), you need to
make sure that find_loader returns None. So what you'd really want to do
is not define dir_found, and later in find_loader, where you've found an
__init__-less directory, say:

    if dir_found is None:
        msg = "Not importing directory {}: missing __init__"
        _warnings.warn(msg.format(dirpath), ImportWarning)
        return None
    dir_found(dirpath)
    return None

> Advantages of this approach:
> - cleanly separates "here's my sys.path entry" (dir_found callback)
> and "please continue scanning sys.path" (None return value)
> - obvious and accurate name for the new method (i.e. "find_loader")
> - provides additional flexibility to import hook consumers
> - trivial to reproduce old API behaviour with the new API if desired

I'm okay with the name. The callback just seems like a hassle,
especially if the finder is written in C. I don't have a problem
specifying the new API as returning a loader, a string, or None.

The only concern I have with returning a string is that it might be
needed in the future for some other purpose. And I don't see any
future-proofing benefit to the callback that isn't provided by just
returning a string.

I don't see the extra flexibility you mention. In my scheme, find_module
would become:

    def find_module(self, fullname):
        result = self.find_loader(fullname)
        if isinstance(result, str):
            msg = "Not importing directory {}: missing __init__"
            _warnings.warn(msg.format(result), ImportWarning)
            result = None
        return result

So I'm unconvinced the callback buys anything. I'm going to change the
PEP to specify find_loader(name) as returning a loader, a string, or
None. And since this only makes sense for a path loader (not a meta-path
loader), it won't have the optional path argument.

Sure, the isinstance call is slightly ugly, but I don't see any downside
outside of aesthetics.

Eric.