New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
inspect.getcomments() does not work in the interactive shell #60559
Comments
The documentation for $ 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)
>>> |
That's because inspecting source code requires that there be a source file to inspect. |
Hmm. Reopening in case someone wants to see if we can generate an appropriate error message when there is no source file to inspect. |
If inspect.getcomments() requires a source file to inspect, I think it would be better to indicate it in the doc. |
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. |
I saw there is the same lack of clarity in the doc of >>> 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 |
Can you elaborate more? |
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. |
Not sure if they can be considered "working" programs if the function fails silently. Do you have some specific example in mind? |
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...) |
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). |
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. |
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. |
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. |
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. |
I pep-8 Phil Connell's work and revamped the unit test based on R. David Murray's request. |
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):
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. |
Only doc fix? What about unit test confirming that getcomments and getsource return None if inspect can not find the source code? |
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. |
Added the doc fix and modified the test. |
"lives in the interactive shell" is not precise; I would prefer "has been defined in ...". |
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. |
So, I just found out that imp has been deprecated. Here is the patch that uses importlib.machinery instead of imp.load_source. |
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 |
Correct. Tests are good, it's the fix I was rejecting. |
Please don't close an issue while there are still open backport PRs on GitHub. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: