Re: PEP 451 (ModuleSpec) round 3

On Wed, Aug 28, 2013 at 4:10 PM, Nick Coghlan <[email protected]> wrote:

> Proper review on the weekend, but quickish comments for now:
>
> - update looks really good, and solves several issues with the original
> proposal. Big +1 for keeping it simple and adding a new finder method :)
>
> - for extension modules that don't define a creation hook (which I won't
> be able to figure out before create_module is called), I'd like to be able
> to return NotImplemented from create_module to say "please give me a normal
> module, or re-use the existing one for reloading".
>
I was wondering about this.  In fact, my current implementation has
create_module() return None as the sentinel.  Would you prefer
NotImplemented?  Either way I'll add that to the PEP.  I was surprised it
wasn't there already.

> - I'd like to finally make "can reload or not" explicit in the loader API.
> My current idea for this is to add a "reloading" parameter to
> create_module, where we pass in the module to be reloaded. Loaders that
> support reloading *must* either not define create_module, or, if they
> define it, return NotImplemented or return the passed in module in that
> case. If it returns a new module, the reload should fail. This shouldn't
> break backwards compatibility, as init based extension modules are cached
> internally, while modules that use the new hooks can decide for themselves
> whether or not to support reloading
>
I'm unclear on how create_module() is involved during reload.  Perhaps the
name has thrown me off, because I understood it as something that happens
only during ModuleSpec.create() and consequently during load().  Isn't the
point to give the loader a chance to do some internal initialization before
the module gets loaded?

I sense that you have a different idea of it.  It does make some sense for
it to take a module rather than being responsible for creating a new one.
 That was probably my own misunderstanding.  However, in that case perhaps
create_module() isn't the right name.  And I'm just not seeing how
create_module() relates to reload().

Regarding the "can reload" point, I was thinking it would simply raise an
ImportError if it can't reload.  Passing in a flag to indicate that the
current call is for a reload makes sense, but I was thinking it would go on
exec_module() rather than create_module().  Of course, I was also thinking
that create_module() wasn't called during reload().

> - I need to check the other proposed reload changes for backwards
> compatibility issues (I'm not sure we can ignore changes made to
> sys.modules in that case)
>
I'm glad you brought this up.  I keep wondering if ModuleSpec.reload()
should assume more of the boilerplate that importlib.reload() has,
including returning whatever is in sys.modules.

> - frozen modules should have a special origin string, too
>
Correct.  It will be "frozen", which matches the current repr.

> - my preferred bikeshed colours are "loader_state" or "loader_info"
>
> Cheers,
> Nick.
>

_______________________________________________
Import-SIG mailing list
[email protected]
http://mail.python.org/mailman/listinfo/import-sig