Re: PEP 451 (ModuleSpec) round 3

On 29 Aug 2013 12:16, "Eric Snow" <[email protected]> wrote:
>
> 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 agree None is a better sentinel for this use case. Just so long as
there is one :)

>> - 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?

If a loader defines create_module, then it is probably the case that
we *shouldn't* allow reloading. Reloading is all about mutating the
existing object in place so that existing references see the changes.
If the loader wants to create a new module every time, that's not
going to be possible, and attempting to reload it should trigger an
exception rather than silently misbehaving (or, worse, crashing the
interpreter if a C extension gets confused).

However, if it returns the same module (as, say, the existing
extension module API would do), then we can go ahead and rerun exec,
knowing that people will see the change.

I agree a "is_reload" flag to exec_module, or a separate can_reload
hook would be clearer, though. Basically, I'd like to get us to a
point where attempting to reload an extension module will either work
as well as it does for a Python module, or will fail with a clear
exception.

>>
>> - 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.

That's the main proposed change that bothered me :)

Cheers,
Nick.