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
bpo-30436: Fix exception raised for invalid parent. #1899
Conversation
Changes `find_spec()` to raise `ModuleNotFoundError` instead of `AttributeError` when parent does not exist or is not a package.
@zvyn, thanks for your PR! By analyzing the history of the files in this pull request, we identified @brettcannon, @ncoghlan and @ericsnowcurrently to be potential reviewers. |
Lib/importlib/util.py
Outdated
parent_path = __import__(parent_name, | ||
fromlist=['__path__']).__path__ | ||
except AttributeError as e: | ||
raise ModuleNotFoundError( |
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.
Should the name
argument be passed?
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.
Yes it should! I'll updated the PR
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.
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. ;)
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.
Please add a Misc/NEWS entry. The rest LGTM.
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.
General idea is good, just some tightening up of some things. And could you also add a .. versionchanged:
note to the docs for importlib.util.find_spec()
about the exception shift?
Once this is all good to go we can add the news and ACKS entries (I'm putting the former off as long as possible as the way to do it might be changing in the near future).
Lib/importlib/util.py
Outdated
parent_path = __import__(parent_name, | ||
fromlist=['__path__']).__path__ | ||
except AttributeError as e: | ||
raise ModuleNotFoundError( |
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.
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. ;)
Lib/importlib/util.py
Outdated
fromlist=['__path__']).__path__ | ||
except AttributeError as e: | ||
raise ModuleNotFoundError( | ||
"Error while finding module specification for '{}'." |
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.
This is going to be a Python 3.7-only change so might as well use an f-string.
Lib/test/test_importlib/test_util.py
Outdated
@@ -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): |
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.
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).
@brettcannon thanks for the review! I committed some changes taking your comments into account |
I made a very minor tweak, @zvyn, of dropping an unnecessary Care to add an entry in Misc/NEWS and yourself to Misc/ACKS (if you're not already there)? |
I'm travelling at the moment, but I might find some time at the airport tomorrow. |
@zvyn no problem. Python 3.7 isn't exactly coming out soon. 😉 Just leave a comment for me when you're done. |
@brettcannon done! I assume |
@zvyn Yes and that issue is being worked on. 😄 I have fixed the conflict by moving your entry down a ways so it's randomly inserted in the appropriate section. I'm now just waiting for status checks to pass again. |
Thanks for the PR! |
Changes
find_spec()
to raiseModuleNotFoundError
instead ofAttributeError
when parent does not exist or is not a package.