Re: PEP 420 issue: extend_path

On Tue, May 8, 2012 at 8:40 PM, Eric V. Smith <[email protected]> wrote:
> 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

No, the idea is to make the two activities (identifying package
portions and deciding whether or not to continue scanning sys.path)
*independent*.

I think "dir_found" is actually the wrong name for the proposed
callback. I would instead call it "portion_found". Also, since this is
an API for third parties to implement, I think the default behaviour
if no callback is specified should be to silently ignore discovered
portions - it would be up to FileFinder.find_module to pass in the
callback with the current ImportWarning behaviour.

Suppose I want to implement a loader where the main path entry is
actually just a reference to a separately configured path definition
(e.g. to an application configuration file with an extra set of paths
to check for Python modules). With a callback API, I can implement
that directly, since I would be able to just pass the received
"portion_found" callback down while scanning the subpath with the
usual sys.path_hooks entries. It doesn't matter if that callback is
called zero, one or many times - it will still do the right thing.

Even if the subscan finds several portions before discovering a
loader, it will *still* do the right thing - the fact we end up
returning return a loader instead of None would override the fact that
we previously called "portion_found".

With the current implementation, there's no option to return
*multiple* path segments - loaders are restricted to returning at most
one portion to add to the namespace package.

I think Antoine's right - having to introspect the return type from
the method call is a major code smell, and I think it's a sign we're
asking one return value to serve too many different purposes.

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

How does a loader request that *multiple* entries be added to the
namespace package path? With the callback API, it can just invokes the
callback multiple times. Introspection on the return type would
instead need another special case.

Regards,
Nick.

-- 
Nick Coghlan   |   [email protected]   |   Brisbane, Australia