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. |
| parent_path = __import__(parent_name, | ||
| fromlist=['__path__']).__path__ | ||
| except AttributeError as e: | ||
| raise ModuleNotFoundError( |
There was a problem hiding this comment.
Should the name argument be passed?
There was a problem hiding this comment.
Yes it should! I'll updated the PR
There was a problem hiding this comment.
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. ;)
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Please add a Misc/NEWS entry. The rest LGTM.
brettcannon
left a comment
There was a problem hiding this comment.
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).
| parent_path = __import__(parent_name, | ||
| fromlist=['__path__']).__path__ | ||
| except AttributeError as e: | ||
| raise ModuleNotFoundError( |
There was a problem hiding this comment.
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.
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
| 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.
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 raiseModuleNotFoundErrorinstead ofAttributeErrorwhen parent does not exist or is not a package.