classification
Title: Wrong name in Lib/pkgutil.py:iter_importers
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ncoghlan Nosy List: berker.peksag, brett.cannon, chris.jerdonek, eric.snow, ezio.melotti, georg.brandl, ncoghlan, python-dev
Priority: high Keywords: patch

Created on 2012-10-08 11:54 by berker.peksag, last changed 2013-04-14 13:02 by ncoghlan. This issue is now closed.

Files
File name Uploaded Description Edit
pkgutil-name-with-test.diff berker.peksag, 2012-10-08 21:07 review
pkgutil-name-with-test_v2.diff berker.peksag, 2012-10-27 16:27 review
pkgutil-name-with-test_v3.diff berker.peksag, 2012-11-04 17:50 review
issue16163_v3.diff berker.peksag, 2013-01-31 17:36 review
Messages (13)
msg172374 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2012-10-08 11:54
Name "pkg" is not defined.

Related changeset: http://hg.python.org/cpython/rev/3987667bf98f#l2.89
msg172387 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-10-08 15:25
Thanks for the report and patch.  Can you also provide a test that fails using the current code (and that passes with the patch applied)?
msg172417 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2012-10-08 21:07
> Can you also provide a test that fails using the current code (and
> that passes with the patch applied)?

New patch attached with a test.
msg173803 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2012-10-25 22:06
Chris, did you have a chance to review the patch I submitted?

The only usage of `iter_importers` function in stdlib is in
`iter_modules` function:

http://hg.python.org/cpython/file/cbdd6852a274/Lib/pkgutil.py#l139
msg173812 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-10-26 00:39
Nick would be a better person to review the patch since IIRC he worked a bunch on that module (e.g. in the changeset you mentioned).  But if I get a chance I will take a look.
msg173935 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2012-10-27 16:27
I've attached a new patch.
msg174688 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-11-03 20:20
I don't think it's necessary to check for UnboundLocalError/NameError in the tests.
msg174822 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2012-11-04 17:50
> I don't think it's necessary to check for UnboundLocalError/NameError
> in the tests.

Thanks for the review.

Attached a new patch.
msg175227 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-11-09 12:21
Given the lack of proper tests for iter_importers, wouldn't adding a proper test that returns something useful, rather than testing only with a non-existent module be better (this test could be kept, maybe converted to an assertRaises)?
msg175229 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-11-09 12:38
OK, I thought that iter_importers was at least being tested indirectly by the tests for walk_packages:
http://hg.python.org/cpython/file/default/Lib/test/test_runpy.py#l417

However, it turns out that iter_modules() only calls iter_importers() if you don't supply an explicit path, and walk_packages() always passes an explicit path down.

I suggest looking at the walk_packages test for ideas on devising a better functional test for iter_importers. Testing iter_modules directly would be good, too.

(Note: There's a series of patches from Chris to move the walk_packages test to where it belongs in test_pkgutil, but they won't be applied until after the final 3.2 maintenance release has happened. Moving the test requires some fairly significant rearranging of the test suite to get the required helper functions out of test_runpy and into a shared, discoverable, location)
msg181038 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2013-01-31 17:36
Sorry for the delay and thanks for the suggestions Ezio and Nick.

> Given the lack of proper tests for iter_importers, wouldn't adding a
> proper test that returns something useful, rather than testing only
> with a non-existent module be better (this test could be kept, maybe
> converted to an assertRaises)?

I've added a test for iter_importers().

> (Note: There's a series of patches from Chris to move the walk_packages
> test to where it belongs in test_pkgutil, but they won't be applied until
> after the final 3.2 maintenance release has happened.)

Thanks for the info. These patches are in issue 15376, issue 15358,
issue 15403 and issue 15415, right?
msg186535 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2013-04-10 23:27
Is there a chance to get this into 3.3.2?
msg186917 - (view) Author: Roundup Robot (python-dev) Date: 2013-04-14 13:01
New changeset c40b50d65a00 by Nick Coghlan in branch '3.3':
Close issue #16163: handle submodules in pkgutil.iter_importers
http://hg.python.org/cpython/rev/c40b50d65a00

New changeset 3bb5a8a4830e by Nick Coghlan in branch 'default':
Merge fix for #16163 from 3.3
http://hg.python.org/cpython/rev/3bb5a8a4830e
History
Date User Action Args
2013-04-14 13:02:13ncoghlansetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2013-04-14 13:01:22python-devsetnosy: + python-dev
messages: + msg186917
2013-04-10 23:27:37berker.peksagsetnosy: + georg.brandl
messages: + msg186535
2013-01-31 17:36:05berker.peksagsetfiles: + issue16163_v3.diff

messages: + msg181038
2012-11-13 02:51:59eric.snowsetnosy: + eric.snow
2012-11-09 12:38:36ncoghlansetmessages: + msg175229
2012-11-09 12:21:55ezio.melottisetmessages: + msg175227
2012-11-04 17:50:33berker.peksagsetfiles: + pkgutil-name-with-test_v3.diff

messages: + msg174822
2012-11-03 20:20:17ezio.melottisetpriority: normal -> high

nosy: + ezio.melotti
messages: + msg174688

type: behavior
stage: patch review
2012-11-03 16:49:57brett.cannonsetnosy: + brett.cannon
2012-11-03 14:27:01ezio.melottilinkissue16393 superseder
2012-10-27 16:28:00berker.peksagsetfiles: - pkgutil-name.diff
2012-10-27 16:27:31berker.peksagsetfiles: + pkgutil-name-with-test_v2.diff

messages: + msg173935
2012-10-26 00:47:42ncoghlansetassignee: ncoghlan
2012-10-26 00:39:04chris.jerdoneksetmessages: + msg173812
2012-10-25 22:06:02berker.peksagsetmessages: + msg173803
2012-10-08 21:07:22berker.peksagsetfiles: + pkgutil-name-with-test.diff

messages: + msg172417
2012-10-08 15:25:16chris.jerdoneksetnosy: + chris.jerdonek
messages: + msg172387
2012-10-08 15:12:20r.david.murraysetnosy: + ncoghlan
2012-10-08 11:54:06berker.peksagcreate