-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
bpo-37173: Show passed class in inspect.getfile error #13861
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
Conversation
Currently, inspect.getfile(str) will report nonsense: >>> inspect.getfile(str) TypeError: <module 'builtins' (built-in)> is a built-in class This fixes that
I think we can skip the news entry on this tiny fix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow how the variable renaming can fix anything.
Please provide a test
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
May I convince you to look again instead? It’s 4 lines of very simple code…
I simply introduce a new variable instead of reassigning |
I agree that the change appears to be is correct, as explained. See the end below. In general, bpo issues get blurbs. A bugfix like this needs a blurb so that anyone who has run into it will see that it is fixed. We are not starved for news space. "The exception message for inspect.getfile() now correctly reports the class rather than module builtins. test_inspect needs 3 new methods added after test_getfile:
Note that test_getfile_builtin_class current fails because the message currently starts with '<module'. The patch should make it pass. |
OK, I was opposed to just add a test for the thing I fixed (basically as a regression test for a trivial change), but proposing that the whole thing ought to be tested makes sense to me. I’ll add both news and tests |
@asvetlov OK, done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Sorry, I can't merge this PR. Reason: |
Thanks @flying-sheep for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8. |
GH-13913 is a backport of this pull request to the 3.8 branch. |
Great! Thank you for the tests @terryjreedy |
Currently, inspect.getfile(str) will report nonsense: ```pytb >>> inspect.getfile(str) TypeError: <module 'builtins' (built-in)> is a built-in class ``` This fixes that https://bugs.python.org/issue37173 (cherry picked from commit d407d2a) Co-authored-by: Philipp A <flying-sheep@web.de>
Thanks @flying-sheep for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7. |
Currently, inspect.getfile(str) will report nonsense: ```pytb >>> inspect.getfile(str) TypeError: <module 'builtins' (built-in)> is a built-in class ``` This fixes that https://bugs.python.org/issue37173 (cherry picked from commit d407d2a) Co-authored-by: Philipp A <flying-sheep@web.de>
GH-13918 is a backport of this pull request to the 3.7 branch. |
@flying-sheep Our general policy is to add an initially failing test, to prevent a regression, for every bugfix. |
Currently, inspect.getfile(str) will report nonsense: ```pytb >>> inspect.getfile(str) TypeError: <module 'builtins' (built-in)> is a built-in class ``` This fixes that https://bugs.python.org/issue37173 (cherry picked from commit d407d2a) Co-authored-by: Philipp A <flying-sheep@web.de>
Currently, inspect.getfile(str) will report nonsense: ```pytb >>> inspect.getfile(str) TypeError: <module 'builtins' (built-in)> is a built-in class ``` This fixes that https://bugs.python.org/issue37173
Currently, inspect.getfile(str) will report nonsense:
This fixes that
https://bugs.python.org/issue37173