Sorry, something went wrong.
There was a problem hiding this comment.
The code removal looks fine, but the doc changes left out the removal of load_module() as well.
Sorry, something went wrong.
|
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again. |
Sorry, something went wrong.
|
@brettcannon: I updated the PR to mention also load_module() removal, and fix test_zipimport... But now, I feel that I removed too many lines in the tests! Maybe the tests should be fixed instead? Problem: I don't know how to fix the packdir2 tests: mod_path = packdir2 + TESTMOD
mod_name = module_path_to_dotted_name(mod_path)
- mod = importlib.import_module(mod_name)
- self.assertTrue(mod_name in sys.modules)
- self.assertIsNone(zi.get_source(TESTPACK))
- self.assertIsNone(zi.get_source(mod_path))
- self.assertEqual(zi.get_filename(mod_path), mod.__file__)
- # To pass in the module name instead of the path, we must use the
- # right importer
- loader = mod.__spec__.loader
- self.assertIsNone(loader.get_source(mod_name))
- self.assertEqual(loader.get_filename(mod_name), mod.__file__)
|
Sorry, something went wrong.
There was a problem hiding this comment.
find_spec() only replaces find_loader() and find_module(), so this sentence is a bit misleading. Same is true for the news entry.
Sorry, something went wrong.
There was a problem hiding this comment.
Why the test removals for lines that don't directly use the associated methods?
Sorry, something went wrong.
|
I don't know how to fix the packdir2 test How is it failing? Maybe only do the find_*() methods in this PR and worry about load_module() in another one? |
Sorry, something went wrong.
|
Maybe only do the find_*() methods in this PR and worry about load_module() in another one? Good idea! I modified this PR to only remove find_loader() and find_module() methods: keep load_module() for now (it can be removed in a separated PR). |
Sorry, something went wrong.
|
@brettcannon: Would you mind to review the updated PR? Now it only removes find_loader() and find_module() methods, and it only removes the bare minimum lines in test_zipimport.py. Once the PR gets approved, I will solve the Doc/whatsnew/3.12.rst conflict. |
Sorry, something went wrong.
|
@vstinner LGTM! And an FYI for anyone else who reviews, the get_filename() function gets used in other places, so it still gets tested. |
Sorry, something went wrong.
|
Thanks for the reviews @warsaw and @brettcannon! It's easier to modify zipimport since @serhiy-storchaka rewrote it in pure Python :-) |
Sorry, something went wrong.
zipimport: Remove find_loader() and find_module() methods, deprecated
in Python 3.10. Use the find_spec()` method instead. See PEP 451 for
the rationale.