8000 bpo-30436: Fix exception raised for invalid parent. by zvyn · Pull Request #1899 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-30436: Fix exception raised for invalid parent. #1899

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.

Sign up for GitHub

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 12 commits into from
Jun 14, 2017
Merged

bpo-30436: Fix exception raised for invalid parent. #1899

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
965834e
bpo-30436: Fix exception raised for invalid parent.
Jun 1, 2017
d0e8069
Add name argument to ModuleNotFoundError call.
Jun 1, 2017
1697a70
Tightening up try-except, docs, cosmetic changes.
Jun 7, 2017
c6e7851
Drop an unnecessary 'f' prefix
brettcannon Jun 7, 2017
c8438b2
Add NEWS entry for bpo-30436.
Jun 14, 2017
6fb807e
Fix merge conflict for NEWS
brettcannon Jun 14, 2017
0723016
Tweak documentation wording
brettcannon Jun 14, 2017
9f2844c
Tweak exception message
brettcannon Jun 14, 2017
9d9c57d
Be very explicit in exception message
brettcannon Jun 14, 2017
6819f9d
Clarify exception message
brettcannon Jun 14, 2017
24fb8e2
Drop a period from the exception message
brettcannon Jun 14, 2017
f6a75fb
Explain what the test is checking for
brettcannon Jun 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions Lib/importlib/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,16 @@ def find_spec(name, package=None):
if fullname not in sys.modules:
parent_name = fullname.rpartition('.')[0]
if parent_name:
# Use builtins.__import__() in case someone replaced it.
parent = __import__(parent_name, fromlist=['__path__'])
return _find_spec(fullname, parent.__path__)
try:
parent_path = __import__(parent_name,
fromlist=['__path__']).__path__
except AttributeError as e:
raise ModuleNotFoundError(
8000
Copy link
Member

Choose a reason for hiding this comment

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

Should the name argument be passed?

Copy link
Contributor Author
@zvyn zvyn Jun 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes it should! I'll updated the PR

Copy link
Member

Choose a reason for hiding this comment

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

Is the AttributeError purely from the attempt to access __path__ on the imported module? If so then the __import__() call should go outside the try block to limit the scope of what will have an AttributeError caught. If there's another potential trigger of the AttributeError then we have a bigger problem. ;)

"Error while finding module specification for '{}'.".format(
fullname)) from e
else:
return _find_spec(fullname, None)
parent_path = None
return _find_spec(fullname, parent_path)
else:
module = sys.modules[fullname]
if module is None:
Expand Down
2 changes: 1 addition & 1 deletion Lib/test/test_cmd_line_script.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ def test_dash_m_errors(self):
tests = (
('builtins', br'No code object available'),
('builtins.x', br'Error while finding module specification.*'
br'AttributeError'),
br'ModuleNotFoundError'),
('builtins.x.y', br'Error while finding module specification.*'
br'ModuleNotFoundError.*No module named.*not a package'),
('os.path', br'loader.*cannot handle'),
Expand Down
9 changes: 9 additions & 0 deletions Lib/test/test_importlib/test_util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,15 @@ def test_find_relative_module_missing_package(self):
self.assertNotIn(name, sorted(sys.modules))
self.assertNotIn(fullname, sorted(sys.modules))

def test_ModuleNotFoundError_raised_for_broken_module_name(self):
Copy link
Member

Choose a reason for hiding this comment

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

It's not that the name is broken, it's that someone tried to import a submodule on something that isn't a package. So a better name might be test_find_submodule_in_module(self).

Also remove the docstring please (unittest prints that out instead of the method name in verbose mode and we prefer the former to make it easier to find the failing test, plus the formatting doesn't follow PEP 8).

"""
Test that calling find_spec with broken name raises ModuleNotFoundError.

See issue 30436 for discussion of this behaviour.
"""
with self.assertRaises(ModuleNotFoundError):
self.util.find_spec('module.name')


(Frozen_FindSpecTests,
Source_FindSpecTests
Expand Down
0