[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Brett Cannon]

Brett Cannon  added the comment:

Do realize the person who created and closed this issue is the one suggesting 
you open a new issue. ;) We can re-evaluate decisions and this one is 5 years 
old. Now I'm not making any promises that we will definitely change anything, 
but I'm not going to swoop in and immediately close it either.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Anthony Sottile]

Anthony Sottile  added the comment:

I suspect it would be closed since it's essentially just a revert of this issue 
:S

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Brett Cannon]

Brett Cannon  added the comment:

Feel free to open a new issue to propose changing it.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Anthony Sottile]

Anthony Sottile  added the comment:

yeah I guess I'm just curious why this bug (seemingly intentionally) made the 
implementation diverge from PEP 451 and require an empty `create_module` (and 
where I would have found that except by searching bugs.python.org)

Everyone I've shown this bit of code has essentially said: "*headscratch* -- 
that's weird?" 
https://github.com/pytest-dev/pytest/blob/6a2d844c5d3e1e692d5bb6fa065c0d753d1dd7d1/src/_pytest/assertion/rewrite.py#L92-L93

why require the function at all I guess

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Brett Cannon]

Brett Cannon  added the comment:

> Mostly looking for something that says how `create_module` should / shouldn't 
> be implemented

Basically you only need to provide the method if you want to use a custom 
object for the module itself. So as long as the object can quack like a module 
object you should be fine.

> (and why `return None` is necessary)

The "return None for default semantics" is there for two reasons (if I remember 
correctly). One, it makes it very easy to implement the default semantics. :) 
Two, it sends a very clear signal that nothing out of the ordinary is going on 
and so you can assume nothing special is expected (which is why LazyLoader 
requires this since it mucks with the object directly in a very specific way).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Anthony Sottile]

Anthony Sottile  added the comment:

Mostly looking for something that says how `create_module` should / shouldn't 
be implemented (and why `return None` is necessary)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Brett Cannon]

Brett Cannon  added the comment:

The import machinery docs are split between the language reference and 
importlib itself. It really depends on what you're looking for.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Anthony Sottile]

Anthony Sottile  added the comment:

Where can I find up to date best practices / docs for the import machinery? I 
couldn't find a mention of this in docs.python.org

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Brett Cannon]

Brett Cannon  added the comment:

PEPs are not living documents unless they are marked as Active (which PEP 451 
is not).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Anthony Sottile]

Anthony Sottile  added the comment:

Hi! just stumbled on this as I was updating pytest's import hook.  I notice in 
PEP 451 this is still marked as optional (then again, I don't know whether PEPs 
are supposed to be living standards or not, or if there was a PEP which 
supersedes that one with better direction?)

--
nosy: +Anthony Sottile

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Brett Cannon]

Changes by Brett Cannon br...@python.org:


--
resolution:  - fixed
stage: patch review - resolved
status: open - closed

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Roundup Robot]

Roundup Robot added the comment:

New changeset ab72f30bcd9f by Brett Cannon in branch 'default':
Issue #23014: Make importlib.abc.Loader.create_module() required when
https://hg.python.org/cpython/rev/ab72f30bcd9f

--
nosy: +python-dev

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Nick Coghlan]

Nick Coghlan added the comment:

I'm still a fan of never - it's a good escape hatch that means we never have 
to contort the new API to deal with some of the more esoteric use cases that 
involved completely overloading the import process in the loader :)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Brett Cannon]

Brett Cannon added the comment:

Yeah, we need to settle the whole load_module() thing at PyCon because I'm 
tired of it hanging over our heads since the code is entirely structured to 
yank it out and if it's going to stay I want to clean up _bootstrap.py to make 
the code flow easier to follow.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Eric Snow]

Eric Snow added the comment:

@Nick

As long as those esoteric cases can be handled without much work via 
create_module and exec_module, we should have a goal in mind for yanking legacy 
load_module support.  If there are valid use cases that can't be handled via 
PEP 451 then I'd say we should consider finding a better way.  If 
Loader.load_module is that better way then we need to consider a better way to 
handle it.

What cases do you have in mind that only load_module handles (at least easily)? 
 I can vaguely imagine the circumstances, but nothing concrete comes to mind.  
It doesn't seem like the sort of situation that has come up more that a handful 
of times nor one the currently couldn't be addressed in another way.  I'm 
probably missing something here and I wouldn't be surprised if we discussed 
this point during the PEP 451 discussions. :)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Eric Snow]

Eric Snow added the comment:

@Brett

Discussing this at PyCon sounds good.  I'm in favor of dropping load_module as 
soon as possible (if possible), as you might have guessed by the way I 
implemented the module spec code. :)  I agree that we should make a decision 
about load_module one way or the other, as either way we can consequently clean 
up that code.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Eric Snow]

Eric Snow added the comment:

Thanks for bringing this up, Brett.  The goal and approach seem good to me.  
Did you bring this up during the PEP 451 discussions?  If so, sorry I missed 
it.  You've made a good point.  And hopefully this will encourage people to 
subclass Loader more. :)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Eric Snow]

Eric Snow added the comment:

At some point should we make create_module (and exec_module) always required?  
In other words, when should would drop the legacy loader support?  I expect the 
answer is Python 4. :)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Brett Cannon]

Brett Cannon added the comment:

I don't think we really need to say anything. If people want default results, 
simply return None (which is handled for them by importlib.abc.Loader). The 
only thing changing here is that the method will now be required instead of 
optional.

I'll post the patch here before I commit to make sure it's all clear.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Brett Cannon]

Brett Cannon added the comment:

Here is a patch that makes sure that if exec_module() is defined then so is 
create_module(). There really isn't any benefit in the code now, but starting 
in Python 3.6 we can make a very clear code path delineation between spec-based 
loading and old-fashioned load_module() instead of the current solution we have 
now where create_module() is used for either path and everything is intertwined.

--
keywords: +patch
stage: test needed - patch review
Added file: http://bugs.python.org/file37427/require_create_module.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Brett Cannon]

Brett Cannon added the comment:

OK, then why don't we just make create_module() a required method to use 
exec_module(), add a DeprecationWarning for when it isn't defined, leave 
Loader.create_module() as-is (already returns None), and then plan to make it a 
requirement in Python 3.6 since the fix is so simple. That leaves the None 
return value for pragmatic reasons but makes it only way to specify the default 
instead of 2.

--
assignee:  - brett.cannon

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Nick Coghlan]

Nick Coghlan added the comment:

Sounds good to me.

I'm not sure what to put in the docs about calling 
importlib.util.module_from_spec() from create_module() implementations. Really, 
if a loader can use that, then it should be doing whatever it was going to do 
after the call in exec_module() instead.

create_module() is intended specifically for cases where you genuinely need to 
do something different to create the object (as in the concept of calling in to 
a C level module creation hook, or the idea of allowing a __new__.py file in 
package directories)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Brett Cannon]

Brett Cannon added the comment:

What do you think about requiring create_module() and/or not supporting a None 
value return? The only reason I ask is to think through the ramifications and 
how long to raise a DeprecationWarning. I think I can live with having a 
reasonable default if create_module() isn't defined, but also supporting None 
seems a bit much. Or I can live with a None value to have import just do the 
most reasonable thing, but you should then explicitly ask for that by returning 
None. Or we just go full-on explicit and require create_module() and that it 
return the object to use.

So assuming we go with the latter, that means people who chose to not subclass 
importlib.abc.Loader will have to now implement create_module(). In Python 3.5 
that's easy since importlib.util.module_from_spec() now exists. But if you're 
supporting older versions it's a bit more work since there was only 
types.ModuleType() and that didn't fill in all attributes since it was missing 
key bits of information that is now (potentially) in the spec. That means if 
someone wants to be compatible from 3.4 on then it will be a bit of work to do 
properly.

That means either a deprecation cycle that is long enough to outlast code that 
wants to work in 3.4 or we don't go as far and support either returning None 
*or* not defining create_module(). My gut says to either deprecate both options 
and make the deprecation last until Python 4 or to require create_module() but 
allow for the returning of None and have that last only a single release since 
adding a create_module() that returns None takes no effort and is fully 
backwards-compatible (my reasoning for the latter is that screwing up to return 
None when you meant a module is easier to debug than accidentally typing the 
method name, plus it means people calling super() won't break in a transition).

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Nick Coghlan]

Nick Coghlan added the comment:

The specific reason I wanted the return None to delegate to the default 
behaviour feature was to make it easier to migrate the C extension machinery.

With that design, a PEP 451 based C extension loader would just need to return 
None when there was no appropriate Create symbol exported from the module.

If returning None was disallowed, it would need to instead arrange to call 
importlib.util.module_from_spec(). That's doable, of course, but requires 
loaders to reimplement behaviour provided by the standard import system, rather 
than being able to just say do the default thing, whatever that happens to 
be. That's the kind of really easy to get wrong responsibility I appreciated 
PEP 451 taking *away* from custom loaders.

However, I have no problem with making create_module() mandatory if the return 
None to request the default behaviour feature is retained. As you say, adding 
an implementation that returns None is both easy and remains compatible with 
Python 3.4.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Brett Cannon]

Brett Cannon added the comment:

Fair enough. I have no qualms with keeping importlib.util.module_from_spec() 
and have Loader.create_module() just call module_from_spec().

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Nick Coghlan]

Nick Coghlan added the comment:

OK, that sounds good to me, then. From a compatibility perspective, a
porting note for 3.5 should cover it.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Arfrever Frehtes Taifersar Arahesis]

Changes by Arfrever Frehtes Taifersar Arahesis arfrever@gmail.com:


--
nosy: +Arfrever

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Brett Cannon]

New submission from Brett Cannon:

I continue to be bothered by how we designed 
importlib.abc.Loader.create_module(). I don't like that it's optional and I 
don't like that it can return None.

I would much rather make it so that importlib.abc.Loader.create_module() was a 
static method that does what importlib.util.module_from_spec() does. I would 
also like importlib to throw a fit if exec_module() was defined on a loader but 
not create_module() with a DeprecationWarning to start and then an 
AttributeError later (as I said, this requires exec_module() defined and does 
not play into the whole load_module() discussion). This should also hopefully 
promote people subclassing importlib.abc.Loader more as well.

What do other people think?

--
components: Interpreter Core
messages: 232313
nosy: brett.cannon, eric.snow, ncoghlan
priority: normal
severity: normal
stage: test needed
status: open
title: Don't have importlib.abc.Loader.create_module() be optional
type: behavior
versions: Python 3.5, Python 3.6

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23014] Don't have importlib.abc.Loader.create_module() be optional

[Nick Coghlan]

Nick Coghlan added the comment:

I mostly like the idea, but am wary of having the base class implement it
as a static method that subclasses are then likely to override with a
normal instance method.

A module level function somewhere + a base class instance method that just
delegates to the stateless function version would feel cleaner to me.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23014
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com