classification
Title: Make importlib.abc.Loader.module_repr optional
Type: Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: barry, brett.cannon, docs@python, eric.smith
Priority: normal Keywords:

Created on 2013-03-28 16:00 by brett.cannon, last changed 2013-04-09 21:05 by brett.cannon. This issue is now closed.

Messages (8)
msg185453 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-03-28 16:00
Various places list explicitly what abstractmethods are needed to make an ABC work in importlib.abc. Unfortunately when module_repr() came into existence it was made abstract but no other documentation was updated to point out this fact.
msg185762 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-04-01 19:40
Barry, Eric: can you clarify why you made module_repr an abstractmethod and thus require its overloading? It seems like its default is fine and you should only need to overload it when you can say something better than the default.
msg185763 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-04-01 19:59
On Apr 01, 2013, at 07:40 PM, Brett Cannon wrote:

>Barry, Eric: can you clarify why you made module_repr an abstractmethod and
>thus require its overloading?

Maybe Eric can, but I can't. ;) I honestly don't remember why we made it
abstract, except perhaps because we put it in the Loader class which already
had an abstract load_module() method.

>It seems like its default is fine and you should only need to overload it
>when you can say something better than the default.

Note that moduleobject.c can handle the case of a missing or uncallable
module_repr() attribute.  Then it implements the defaults specified in PEP
420.
msg185770 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-04-01 21:04
If Eric doesn't have anything to add then I would like to change importlib.abc.Loader.module_repr() to no longer be abstract and the default to be defined as ``return repr(module)``. Else it should be entirely optional and not have a default implementation, but having it be required as abstract doesn't seem quite right to me.
msg185772 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2013-04-01 21:35
I was going to blame Barry, but I see he beat me to it :)

It looks like an oversight, and it shouldn't be abstract.
msg185775 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-04-01 21:55
On Apr 01, 2013, at 09:04 PM, Brett Cannon wrote:

>If Eric doesn't have anything to add then I would like to change
>importlib.abc.Loader.module_repr() to no longer be abstract and the default
>to be defined as ``return repr(module)``. Else it should be entirely optional
>and not have a default implementation, but having it be required as abstract
>doesn't seem quite right to me.

I have no problem making it non-abstract, but I don't think you need to define
a default.  The built-in module object already does the right thing by
default.  Maybe that's why we (mistakenly?) made it abstract.  We might have
hoped to document the API but not provide a default implementation, since if
module_repr() is not both defined and callable, moduleobject.c already does
the right thing.

But at least there are unit tests so it should be hard for you to break
things. :)
msg185778 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-04-01 22:22
Totally optional and no default argument it is.
msg186450 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-04-09 21:05
Fixed by http://hg.python.org/cpython/rev/8e733e30edf6 by raising NotImplementedError and relying on the fact that ModuleType.__repr__() swallows any exception.
History
Date User Action Args
2013-04-09 21:05:22brett.cannonsetstatus: open -> closed
resolution: fixed
messages: + msg186450

stage: needs patch -> resolved
2013-04-09 19:29:28brett.cannonsettitle: Document that importlib.abc.Loader.module_repr is abstract and thus needed by various other ABCs -> Make importlib.abc.Loader.module_repr optional
components: + Library (Lib), - Documentation
versions: - Python 3.3
2013-04-01 22:22:45brett.cannonsetassignee: docs@python -> brett.cannon
messages: + msg185778
2013-04-01 21:55:25barrysetmessages: + msg185775
2013-04-01 21:35:35eric.smithsetmessages: + msg185772
2013-04-01 21:04:35brett.cannonsetmessages: + msg185770
2013-04-01 19:59:22barrysetmessages: + msg185763
2013-04-01 19:40:19brett.cannonsetnosy: + barry, eric.smith
messages: + msg185762
2013-03-28 16:00:22brett.cannoncreate