bpo-30436: Fix exception raised for invalid parent. by zvyn · Pull Request #1899 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits12 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Changes find_spec() to raise ModuleNotFoundError instead of
AttributeError when parent does not exist or is not a package.

Changes find_spec() to raise ModuleNotFoundError instead of AttributeError when parent does not exist or is not a package.

parent_path = __import__(parent_name,
fromlist=['__path__']).__path__
except AttributeError as e:
raise ModuleNotFoundError(

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?

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~~ update_d_ the PR

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. ;)

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.

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).

parent_path = __import__(parent_name,
fromlist=['__path__']).__path__
except AttributeError as e:
raise ModuleNotFoundError(

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. ;)

fromlist=['__path__']).__path__
except AttributeError as e:
raise ModuleNotFoundError(
"Error while finding module specification for '{}'."

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.

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

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 f prefix on a string, but otherwise the code changes LGTM!

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 Misc/NEWS will be out of date the minute I merge so I'll leave that to you :)

@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.