classification
Title: inspect.getcomments() does not work in the interactive shell
Type: behavior Stage: resolved
Components: Documentation, Library (Lib) Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: docs@python Nosy List: berker.peksag, docs@python, ezio.melotti, georg.brandl, marco.buttu, pconnell, r.david.murray, vajrasky
Priority: normal Keywords: easy, patch

Created on 2012-10-29 11:58 by marco.buttu, last changed 2017-03-24 22:15 by berker.peksag. This issue is now closed.

Files
File name Uploaded Description Edit
issue16355.diff pconnell, 2013-04-18 21:03 review
issue16355_v2.diff vajrasky, 2013-10-06 15:18 Based on Phil Connell's work (revamped unit test and pep-8) review
issue16355_v3.diff vajrasky, 2013-10-12 04:35 review
issue16355_v4.diff vajrasky, 2013-10-13 03:52 review
issue16355_v5.diff vajrasky, 2013-12-14 06:55 review
Pull Requests
URL Status Linked Edit
PR 428 merged marco.buttu, 2017-03-03 14:02
PR 690 closed berker.peksag, 2017-03-17 08:55
PR 691 merged berker.peksag, 2017-03-17 11:37
Messages (31)
msg174108 - (view) Author: Marco Buttu (marco.buttu) * Date: 2012-10-29 11:58
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)
>>>
msg174111 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-10-29 12:24
That's because inspecting source code requires that there be a source file to inspect.
msg174112 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-10-29 12:25
Hmm.  Reopening in case someone wants to see if we can generate an appropriate error message when there is no source file to inspect.
msg174113 - (view) Author: Marco Buttu (marco.buttu) * Date: 2012-10-29 12:34
If inspect.getcomments() requires a source file to inspect, I think it would be better to indicate it in the doc.
msg174119 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-10-29 15:01
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.
msg174120 - (view) Author: Marco Buttu (marco.buttu) * Date: 2012-10-29 15:19
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
msg174122 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-10-29 16:07
> 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.
msg174125 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-10-29 16:28
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.
msg174133 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-10-29 17:06
Not sure if they can be considered "working" programs if the function fails silently.  Do you have some specific example in mind?
msg174134 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-10-29 17:18
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...)
msg174738 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-11-04 01:43
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).
msg174755 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-11-04 05:00
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.
msg174758 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-11-04 05:07
> 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.
msg187293 - (view) Author: Phil Connell (pconnell) * Date: 2013-04-18 21:03
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.
msg193954 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-07-30 20:29
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.
msg199082 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-10-06 15:18
I pep-8 Phil Connell's work and revamped the unit test based on  R. David Murray's request.
msg199340 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-10-09 20:04
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.
msg199395 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-10-10 15:57
Only doc fix? What about unit test confirming that getcomments and getsource return None if inspect can not find the source code?
msg199397 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-10-10 16:14
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.
msg199537 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-10-12 04:35
Added the doc fix and modified the test.
msg199590 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2013-10-12 16:24
"lives in the interactive shell" is not precise; I would prefer "has been defined in ...".
msg199653 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-10-13 03:52
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.
msg206170 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-12-14 06:55
So, I just found out that imp has been deprecated. Here is the patch that uses importlib.machinery instead of imp.load_source.
msg288486 - (view) Author: Marco Buttu (marco.buttu) * Date: 2017-02-23 21:32
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
msg288888 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-03-03 14:20
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.
msg288892 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2017-03-03 14:43
Correct.  Tests are good, it's the fix I was rejecting.
msg289755 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-03-17 11:24
Please don't close an issue while there are still open backport PRs on GitHub.
msg289756 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-03-17 12:01
Now all PRs have been merged:

* master: https://github.com/python/cpython/commit/3f2155ffe683080f2a1b28408fa48d43ba92f943
* 3.6: https://github.com/python/cpython/commit/948171bf999cf8b3e12048851041d2e04ae3a78c
* 3.5: https://github.com/python/cpython/commit/41b4a2189f29daae008e57f799a30890643d191f

Thanks for the patches, Vajrasky and Marco!
msg290172 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-03-24 22:14
New changeset 948171bf999cf8b3e12048851041d2e04ae3a78c by Berker Peksag in branch '3.6':
bpo-16355: Clarify when inspect.getcomments() returns None (#428) (#690)
https://github.com/python/cpython/commit/948171bf999cf8b3e12048851041d2e04ae3a78c
msg290173 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-03-24 22:14
New changeset 41b4a2189f29daae008e57f799a30890643d191f by Berker Peksag in branch '3.5':
bpo-16355: Clarify when inspect.getcomments() returns None (#428) (#691)
https://github.com/python/cpython/commit/41b4a2189f29daae008e57f799a30890643d191f
msg290174 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-03-24 22:15
New changeset 3f2155ffe683080f2a1b28408fa48d43ba92f943 by Berker Peksag (Marco Buttu) in branch 'master':
bpo-16355: Clarify when inspect.getcomments() returns None  (#428)
https://github.com/python/cpython/commit/3f2155ffe683080f2a1b28408fa48d43ba92f943
History
Date User Action Args
2017-03-24 22:15:43berker.peksagsetmessages: + msg290174
2017-03-24 22:14:19berker.peksagsetmessages: + msg290173
2017-03-24 22:14:12berker.peksagsetmessages: + msg290172
2017-03-17 12:01:35berker.peksagsetstatus: open -> closed
resolution: fixed
messages: + msg289756

stage: resolved
2017-03-17 11:37:53berker.peksagsetpull_requests: + pull_request567
2017-03-17 11:24:26berker.peksagsetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg289755

stage: resolved -> (no value)
2017-03-17 10:33:43marco.buttusetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-03-17 08:55:12berker.peksagsetpull_requests: + pull_request565
2017-03-03 14:43:39r.david.murraysetmessages: + msg288892
2017-03-03 14:20:33berker.peksagsetnosy: + berker.peksag

messages: + msg288888
versions: + Python 3.5, Python 3.6, Python 3.7, - Python 3.3, Python 3.4
2017-03-03 14:02:04marco.buttusetpull_requests: + pull_request356
2017-02-23 21:32:14marco.buttusetmessages: + msg288486
2013-12-14 06:55:45vajraskysetfiles: + issue16355_v5.diff

messages: + msg206170
2013-10-13 03:52:40vajraskysetfiles: + issue16355_v4.diff

messages: + msg199653
2013-10-12 16:24:10georg.brandlsetnosy: + georg.brandl
messages: + msg199590
2013-10-12 04:35:19vajraskysetfiles: + issue16355_v3.diff

messages: + msg199537
2013-10-10 16:14:22r.david.murraysetmessages: + msg199397
2013-10-10 15:57:21vajraskysetmessages: + msg199395
2013-10-09 20:04:10r.david.murraysetmessages: + msg199340
2013-10-06 15:18:33vajraskysetfiles: + issue16355_v2.diff
nosy: + vajrasky
messages: + msg199082

2013-07-30 20:29:30r.david.murraysetmessages: + msg193954
2013-04-19 03:51:25ezio.melottisetstage: needs patch -> patch review
versions: - Python 3.2
2013-04-18 21:03:27pconnellsetfiles: + issue16355.diff

nosy: + pconnell
messages: + msg187293

keywords: + patch
2012-11-04 05:07:39ezio.melottisetmessages: + msg174758
2012-11-04 05:00:58r.david.murraysetmessages: + msg174755
2012-11-04 01:43:41ezio.melottisetmessages: + msg174738
2012-10-29 17:18:29r.david.murraysetmessages: + msg174134
2012-10-29 17:06:55ezio.melottisetmessages: + msg174133
2012-10-29 16:28:13r.david.murraysetmessages: + msg174125
2012-10-29 16:07:39ezio.melottisettype: enhancement -> behavior
resolution: not a bug -> (no value)
messages: + msg174122
components: + Library (Lib)
2012-10-29 15:19:17marco.buttusetmessages: + msg174120
2012-10-29 15:09:00ezio.melottisetversions: + Python 3.2
nosy: + ezio.melotti

keywords: + easy
type: behavior -> enhancement
stage: resolved -> needs patch
2012-10-29 15:01:57r.david.murraysetnosy: + docs@python
messages: + msg174119

assignee: docs@python
components: + Documentation, - Library (Lib)
2012-10-29 12:34:10marco.buttusetmessages: + msg174113
2012-10-29 12:25:17r.david.murraysetstatus: closed -> open

messages: + msg174112
versions: + Python 2.7, Python 3.4
2012-10-29 12:24:09r.david.murraysetstatus: open -> closed

nosy: + r.david.murray
messages: + msg174111

resolution: not a bug
stage: resolved
2012-10-29 11:58:50marco.buttucreate