Skip to content

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

Merged
merged 3 commits into from
Jun 8, 2019

Conversation

flying-sheep
Copy link
Contributor

@flying-sheep flying-sheep commented Jun 6, 2019

Currently, inspect.getfile(str) will report nonsense:

>>> 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:

>>> inspect.getfile(str)
TypeError: <module 'builtins' (built-in)> is a built-in class

This fixes that
@flying-sheep
Copy link
Contributor Author

I think we can skip the news entry on this tiny fix

Copy link
Contributor

@asvetlov asvetlov left a 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

@bedevere-bot
Copy link

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@flying-sheep
Copy link
Contributor Author

May I convince you to look again instead? It’s 4 lines of very simple code…

  1. We go into the branch when object is a class
  2. object gets reassigned to the class’ module
  3. An error gets thrown, with the message talking about the class.

I simply introduce a new variable instead of reassigning object, so it stays assigned to the class and the error message is correct.

@terryjreedy
Copy link
Member

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:

    def test_getfile_builtin_module(self):
        with self.assertRaises(TypeError) as e:
            inspect.getfile(sys)
        self.assertTrue(str(e.exception).startswith('<module'))

    def test_getfile_builtin_class(self):
        with self.assertRaises(TypeError) as e:
            inspect.getfile(int)
        self.assertTrue(str(e.exception).startswith('<class'))

    def test_getfile_builtin_function_or_method(self):
        with self.assertRaises(TypeError) as e:
            inspect.getfile(abs)
        self.assertIn('expected, got', str(e.exception))
        with self.assertRaises(TypeError) as e:
            inspect.getfile(list.append)
        self.assertIn('expected, got', str(e.exception))

Note that test_getfile_builtin_class current fails because the message currently starts with '<module'. The patch should make it pass.

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Jun 8, 2019

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

@flying-sheep
Copy link
Contributor Author

@asvetlov OK, done

Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@miss-islington
Copy link
Contributor

Sorry, I can't merge this PR. Reason: Base branch was modified. Review and try the merge again..

@miss-islington miss-islington merged commit d407d2a into python:master Jun 8, 2019
@miss-islington
Copy link
Contributor

Thanks @flying-sheep for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-13913 is a backport of this pull request to the 3.8 branch.

@flying-sheep
Copy link
Contributor Author

Great! Thank you for the tests @terryjreedy

miss-islington added a commit that referenced this pull request Jun 8, 2019
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>
@miss-islington
Copy link
Contributor

Thanks @flying-sheep for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 8, 2019
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>
@bedevere-bot
Copy link

GH-13918 is a backport of this pull request to the 3.7 branch.

@terryjreedy
Copy link
Member

@flying-sheep Our general policy is to add an initially failing test, to prevent a regression, for every bugfix.

miss-islington added a commit that referenced this pull request Jun 8, 2019
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>
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants