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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2017-03-03 14:43 |
Correct. Tests are good, it's the fix I was rejecting.
|
msg289755 - (view) |
Author: Berker Peksag (berker.peksag) * |
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) * |
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) * |
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) * |
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) * |
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
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:37 | admin | set | github: 60559 |
2017-03-24 22:15:43 | berker.peksag | set | messages:
+ msg290174 |
2017-03-24 22:14:19 | berker.peksag | set | messages:
+ msg290173 |
2017-03-24 22:14:12 | berker.peksag | set | messages:
+ msg290172 |
2017-03-17 12:01:35 | berker.peksag | set | status: open -> closed resolution: fixed messages:
+ msg289756
stage: resolved |
2017-03-17 11:37:53 | berker.peksag | set | pull_requests:
+ pull_request567 |
2017-03-17 11:24:26 | berker.peksag | set | status: closed -> open resolution: fixed -> (no value) messages:
+ msg289755
stage: resolved -> (no value) |
2017-03-17 10:33:43 | marco.buttu | set | status: open -> closed resolution: fixed stage: patch review -> resolved |
2017-03-17 08:55:12 | berker.peksag | set | pull_requests:
+ pull_request565 |
2017-03-03 14:43:39 | r.david.murray | set | messages:
+ msg288892 |
2017-03-03 14:20:33 | berker.peksag | set | nosy:
+ 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:04 | marco.buttu | set | pull_requests:
+ pull_request356 |
2017-02-23 21:32:14 | marco.buttu | set | messages:
+ msg288486 |
2013-12-14 06:55:45 | vajrasky | set | files:
+ issue16355_v5.diff
messages:
+ msg206170 |
2013-10-13 03:52:40 | vajrasky | set | files:
+ issue16355_v4.diff
messages:
+ msg199653 |
2013-10-12 16:24:10 | georg.brandl | set | nosy:
+ georg.brandl messages:
+ msg199590
|
2013-10-12 04:35:19 | vajrasky | set | files:
+ issue16355_v3.diff
messages:
+ msg199537 |
2013-10-10 16:14:22 | r.david.murray | set | messages:
+ msg199397 |
2013-10-10 15:57:21 | vajrasky | set | messages:
+ msg199395 |
2013-10-09 20:04:10 | r.david.murray | set | messages:
+ msg199340 |
2013-10-06 15:18:33 | vajrasky | set | files:
+ issue16355_v2.diff nosy:
+ vajrasky messages:
+ msg199082
|
2013-07-30 20:29:30 | r.david.murray | set | messages:
+ msg193954 |
2013-04-19 03:51:25 | ezio.melotti | set | stage: needs patch -> patch review versions:
- Python 3.2 |
2013-04-18 21:03:27 | pconnell | set | files:
+ issue16355.diff
nosy:
+ pconnell messages:
+ msg187293
keywords:
+ patch |
2012-11-04 05:07:39 | ezio.melotti | set | messages:
+ msg174758 |
2012-11-04 05:00:58 | r.david.murray | set | messages:
+ msg174755 |
2012-11-04 01:43:41 | ezio.melotti | set | messages:
+ msg174738 |
2012-10-29 17:18:29 | r.david.murray | set | messages:
+ msg174134 |
2012-10-29 17:06:55 | ezio.melotti | set | messages:
+ msg174133 |
2012-10-29 16:28:13 | r.david.murray | set | messages:
+ msg174125 |
2012-10-29 16:07:39 | ezio.melotti | set | type: enhancement -> behavior resolution: not a bug -> (no value) messages:
+ msg174122 components:
+ Library (Lib) |
2012-10-29 15:19:17 | marco.buttu | set | messages:
+ msg174120 |
2012-10-29 15:09:00 | ezio.melotti | set | versions:
+ Python 3.2 nosy:
+ ezio.melotti
keywords:
+ easy type: behavior -> enhancement stage: resolved -> needs patch |
2012-10-29 15:01:57 | r.david.murray | set | nosy:
+ docs@python messages:
+ msg174119
assignee: docs@python components:
+ Documentation, - Library (Lib) |
2012-10-29 12:34:10 | marco.buttu | set | messages:
+ msg174113 |
2012-10-29 12:25:17 | r.david.murray | set | status: closed -> open
messages:
+ msg174112 versions:
+ Python 2.7, Python 3.4 |
2012-10-29 12:24:09 | r.david.murray | set | status: open -> closed
nosy:
+ r.david.murray messages:
+ msg174111
resolution: not a bug stage: resolved |
2012-10-29 11:58:50 | marco.buttu | create | |