inspect.getcomments() does not work in the interactive shell

[marco-buttu]

BPO	16355
Nosy	@birkenfeld, @ezio-melotti, @bitdancer, @berkerpeksag, @marco-buttu, @phmc, @vajrasky
PRs	bpo-16355: fix a lack in inspect.getcomments() doc #428 [3.6] bpo-16355: Clarify when inspect.getcomments() returns None (#428) #690 [3.5] bpo-16355: Clarify when inspect.getcomments() returns None (#428) #691
Files	issue16355.diff issue16355_v2.diff: Based on Phil Connell's work (revamped unit test and pep-8) issue16355_v3.diff issue16355_v4.diff issue16355_v5.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2017-03-17.12:01:35.130>
created_at = <Date 2012-10-29.11:58:50.804>
labels = ['3.7', 'easy', 'type-bug', 'library', 'docs']
title = 'inspect.getcomments() does not work in the interactive shell'
updated_at = <Date 2017-03-24.22:15:43.087>
user = 'https://github.com/marco-buttu'

bugs.python.org fields:

activity = <Date 2017-03-24.22:15:43.087>
actor = 'berker.peksag'
assignee = 'docs@python'
closed = True
closed_date = <Date 2017-03-17.12:01:35.130>
closer = 'berker.peksag'
components = ['Documentation', 'Library (Lib)']
creation = <Date 2012-10-29.11:58:50.804>
creator = 'marco.buttu'
dependencies = []
files = ['29926', '31975', '32057', '32073', '33127']
hgrepos = []
issue_num = 16355
keywords = ['patch', 'easy']
message_count = 31.0
messages = ['174108', '174111', '174112', '174113', '174119', '174120', '174122', '174125', '174133', '174134', '174738', '174755', '174758', '187293', '193954', '199082', '199340', '199395', '199397', '199537', '199590', '199653', '206170', '288486', '288888', '288892', '289755', '289756', '290172', '290173', '290174']
nosy_count = 8.0
nosy_names = ['georg.brandl', 'ezio.melotti', 'r.david.murray', 'docs@python', 'berker.peksag', 'marco.buttu', 'pconnell', 'vajrasky']
pr_nums = ['428', '690', '691']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue16355'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6', 'Python 3.7']

[marco-buttu]

The documentation for inspect.getcomments() says that it returns
the "lines of comments immediately preceding an object's source code".
It works fine for the comments that immediately preceded an object
defined in a module:

$ more foo.py 
import inspect
# A dummy comment
def foo():
    pass

print(inspect.getcomments(foo))
$ python3.3 foo.py 
# A dummy comment

But it does not work if we define an object interactively:

$ python3.3
Python 3.3.0 (default, Oct  9 2012, 18:20:32) 
[GCC 4.5.2] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import inspect
>>> # A dummy comment
... def foo():
...     pass
... 
>>> inspect.getcomments(foo)
>>> # A dummy comment
... 
>>> def foo():
...     pass
... 
>>> inspect.getcomments(foo)
>>>

[bitdancer]

That's because inspecting source code requires that there be a source file to inspect.

[bitdancer]

Hmm. Reopening in case someone wants to see if we can generate an appropriate error message when there is no source file to inspect.

[marco-buttu]

If inspect.getcomments() requires a source file to inspect, I think it would be better to indicate it in the doc.

[bitdancer]

Yeah, this should be a doc bug. After I thought about it some more I realized that changing the behavior to raise an error would not be a good idea.

[marco-buttu]

I saw there is the same lack of clarity in the doc of inspect.getsource():

>>> import inspect
>>> print(inspect.getsource.__doc__)
Return the text of the source code for an object.

    The argument may be a module, class, method, function, traceback, frame,
    or code object.  The source code is returned as a single string.  An
    IOError is raised if the source code cannot be retrieved.
>>> def foo():
...     pass
... 
>>> inspect.getsource(foo)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.3/inspect.py", line 726, in getsource
    lines, lnum = getsourcelines(object)
  File "/usr/local/lib/python3.3/inspect.py", line 715, in getsourcelines
    lines, lnum = findsource(object)
  File "/usr/local/lib/python3.3/inspect.py", line 563, in findsource
    raise IOError('could not get source code')
OSError: could not get source code

[ezio-melotti]

After I thought about it some more I realized that changing
the behavior to raise an error would not be a good idea.

Can you elaborate more?
Failing silently doesn't seem much better than raising an error, especially if similar functions already raise.

[bitdancer]

If similar functions already raise, then changing it for 3.4 would be OK. But to change it for earlier versions would risk breaking working programs.

[ezio-melotti]

Not sure if they can be considered "working" programs if the function fails silently. Do you have some specific example in mind?

[bitdancer]

Not a specific package, but a specific use case (assuming getcomments is in use at all :)

Consider a program that uses getcomments to look for a pragma-like comment before a function, but one that is not critical to the program's function (perhaps it has to do with testing or tracing infrastructure...). If the source file does not exist, then that is equivalent to their being no pragma. So the program would work fine even if the source is deleted...until this getcomments starts to raise an error, when it promptly crashes (probably in a production system, since that is the most likely place for source to be absent...)

[ezio-melotti]

That's arguably a bug though. If the pragma was critical the program will silently do the wrong thing by ignoring an existing pragma when the source is missing (e.g. in production, even after passing all the tests locally).

[bitdancer]

It doesn't matter whether it is a bug or not (though it is not in the situation I described). The point is that a working program would stop working. That is the kind of breakage our backward compatibility policy is designed to protect against.

[ezio-melotti]

The point is that a working program would stop working.

My point is that the program might not be a "working" program, if the function fails silently.

Anyway this function is probably not widely used, so I'm fine either ways.

[phmc]

Here's a patch that updates getcomments to match the behaviour of getsource, raising OSError if the source file can't be found and TypeError when passed a built-in.

Since this is a backwards-incompatible change, presumably it can only be applied to 3.4.

This is ready for review.

[bitdancer]

The patch looks good, but I would prefer that the test not use mock+knowledge of how get_comment is implemented, but instead test the public API by actually creating a module and deleting the source file. test.script_helpers can be used to do this easily.

[vajrasky]

I pep-8 Phil Connell's work and revamped the unit test based on R. David Murray's request.

[bitdancer]

Looking at the source, the suppression of errors is clearly intentional. Looking at the change that added the TypeError check, we see this from Jeremy Hilton in March 2002 (9c2ca37bdeec):

It appears that getcomments() can get called for classes defined in
C.  Since these don't have source code, it can't do anything useful.
A function buried many levels deep was raising a TypeError that was
not caught.

Who knows why this broke...

Which implies that getcomments was being called from somewhere in Python itself...at least back then.

The check for OSError (IOError, then) was from shortly after the module was first added, in February of 2001 by Ka-Ping Yee.

So, the motivation behind this behavior are shrouded in the mists of time :)

Should we really be changing something of that long standing, when it raising a TypeError previously clearly broke something?

I'm thinking not. I'm thinking we should leave well enough alone. It is, after all, working as documented....in the doc string, from the time Jeremy made the TypeError change. So after all this work (sorry people, I do appreciate the work!), I think we should just make a doc fix that copies the line about returning None if source can't be found into the rst docs.

[vajrasky]

Only doc fix? What about unit test confirming that getcomments and getsource return None if inspect can not find the source code?

[bitdancer]

Yes, good point, those tests should definitely be added, which means the test work doesn't go to waste :)

Note, however, that getsource *does* raise.

[vajrasky]

Added the doc fix and modified the test.

[birkenfeld]

"lives in the interactive shell" is not precise; I would prefer "has been defined in ...".

[vajrasky]

Attached the patch to address Georg Brandl's concern (Thank you!). I also added test for checking the comment of the object defined in C (list, open, etc). I have given thought about testing the comment of the object in interactive shell. But it is too much hassle. So I skip it.

[vajrasky]

So, I just found out that imp has been deprecated. Here is the patch that uses importlib.machinery instead of imp.load_source.

[marco-buttu]

Hello Vajrasky, the doc patch LGTM. Looking at the David's comment in Rietveld, it seems that he does not want the test patch to be applyed. Can you please make a pull request? Thank you very much

[berkerpeksag]

I think David's comment about tests was for the second patch. Looking at msg199395 and msg199397, my understanding is that tests are still needed.

[bitdancer]

Correct. Tests are good, it's the fix I was rejecting.

[berkerpeksag]

Please don't close an issue while there are still open backport PRs on GitHub.

[berkerpeksag]

Now all PRs have been merged:

• master: 3f2155f
• 3.6: 948171b
• 3.5: 41b4a21

Thanks for the patches, Vajrasky and Marco!

[berkerpeksag]

New changeset 948171b by Berker Peksag in branch '3.6':
bpo-16355: Clarify when inspect.getcomments() returns None (#428) (#690)
948171b

[berkerpeksag]

New changeset 41b4a21 by Berker Peksag in branch '3.5':
bpo-16355: Clarify when inspect.getcomments() returns None (#428) (#691)
41b4a21

[berkerpeksag]

New changeset 3f2155f by Berker Peksag (Marco Buttu) in branch 'master':
bpo-16355: Clarify when inspect.getcomments() returns None (#428)
3f2155f