bpo-24744: Raises error in pkgutil.walk_packages if path is str by CuriousLearner · Pull Request #1926 · python/cpython

CuriousLearner · 2017-06-03T07:58:23Z

Attempts to resolve: http://bugs.python.org/issue24744

In the Bug @bitdancer suggested that some sort of error should be raised if path is of type str in pkgutil.walk_packages function.

@bitdancer For the meanwhile I've raised TypeError since that was looking most appropriate to me. Please let me know what kind of error is expected to be raised here and I'll update the PR.

Thanks!

mention-bot · 2017-06-03T07:58:25Z

@CuriousLearner, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Yhg1s, @ericsnowcurrently and @brettcannon to be potential reviewers.

bitdancer · 2017-06-03T13:45:24Z

I think ValueError would be better, but it doesn't matter all that much (the two have a very blurry line between them in Python).

A test is needed as well. And we should also add a test that a bytes object also produces some sort of error (we don't care what sort).

CuriousLearner · 2017-06-03T15:49:49Z

@bitdancer what would be the most appropriate place to put the test at?

I can see that there is a test written for pkgutil.walk_packages in Lib/test/test_runner.py by @ncoghlan . Would that file be a good place to add this test?

bitdancer · 2017-06-03T18:23:47Z

I haven't reviewed the reason for the 'hack' of putting the walk_packages test in test_runner, but since this is only checking the raising of an error on bad input, I'm guessing you can put it in test_pkgutil. Try it :) The long term goal is to move the tests from test_runner to test_pkgutil, so any new tests that can go in test_pkgutil should go there.

CuriousLearner · 2017-06-03T19:31:24Z

Hi @bitdancer, I've updated the branch with changes and also added a test.

However, I'm not sure why the PR is not reflecting it, although it's from the same branch (https://github.com/CuriousLearner/cpython/tree/fix-issue24744) I'm doing the changes in. This seems weird to me.

CuriousLearner · 2017-06-03T19:41:38Z

Never mind, I figured that out, it was a minor naming issue with the branch. The patch would be now updated here.

Please have a look @bitdancer . Let me know if this needs changes.

Thanks!

ncoghlan · 2017-06-04T02:47:03Z

@bitdancer Some of the tests currently in test_runpy are there not because that's the right place for them, but because they re-use some of the testing machinery that's currently in that file. There are a couple of old issues about moving those helpers out into the test.support subpackage, allowing the affected test cases to be moved to test_pkgutil where they belong:

Moving the walk_packages helpers out of test_runpy: https://bugs.python.org/issue15376
Moving the sample package creation helpers to a common location: https://bugs.python.org/issue15403

bitdancer · 2017-06-04T23:08:02Z

+        with self.assertRaises(ValueError) as context:
+            list(pkgutil.walk_packages(str_input))
+
+        self.assertTrue('path must be None or list of paths' in str(context.exception))


I don't think this assertion is needed. All we care about here is that the ValueError gets raised, we really don't care what the error message is. (This is not always true, but in this case I think it is.)

In fact, in this particular case we don't even care whether we get a ValueError or a TypeError, so I'd make the test accept both as passing. That way we're doing the same check for both str and bytes: all we are trying to prove with these tests is that we get an error rather than an empty list.

bitdancer · 2017-06-04T23:11:00Z

    """
    if path is None:
        importers = iter_importers()
+    elif isinstance(path, str) or isinstance(path, bytes):


isinstance(path, bytes) is problematic. I was hoping that walk_packages would throw some other error for bytes input (perhaps a TypeError on some string operation), because checking for all the bytes types is a bit of a pain. And testing it, this is in fact true. So the bytes check isn't needed, just the str check. (Yes, the error is a bit mysterious, but if we want to address that, it requires more of a design discussion, and we shouldn't deal with that in this particular issue).

Hi @bitdancer , what is meant by:

checking for all the bytes types

bytes, bytearray, array('b') (or whatever the right incantation is), memoryview....

bitdancer · 2017-06-05T19:44:17Z

    """
    if path is None:
        importers = iter_importers()
+    elif isinstance(path, str) or isinstance(path, bytes):


bytes, bytearray, array('b') (or whatever the right incantation is), memoryview....

bitdancer · 2017-06-05T19:46:16Z

-
        bytes_input = b'test_dir'
-        with self.assertRaises(ValueError) as context:
+        with self.assertRaises(TypeError):


I would like both of these to be assertRaises((TypeError, ValueError)).

@bitdancer Fixed. Does this looks good now?

bitdancer · 2017-06-06T12:41:42Z

It would be better if the two tuples were in the same order, but I wouldn't have bothered you with that except that I'd like to request that you delete the test method docstring. We don't in general use docstrings for test methods in the cpython test suite. If there is important information not included in the test name, you can use a comment. But I'd just rename the test "test_walk_packages_raises_on_string_or_bytes_input".

CuriousLearner · 2017-06-06T12:46:19Z

Sure @bitdancer , I've done the changes :)

bitdancer

I don't see the NEWS entry, I suspect you forgot to 'git add' it.

bitdancer · 2017-06-06T13:44:00Z

+* :meth:`pkgutil.walk_packages` now raises ValueError/TypeError if path is
+  string/bytes object.
+  Previously an empty list was returned. (Contributed by Sanyam Khurana in
+  :issue:`24744`.)


Please reflow this into a single paragraph. And it should only mention str; it has always raised a TypeError for bytes.

bitdancer · 2017-06-06T17:07:33Z

-  :issue:`24744`.)
+* :meth:`pkgutil.walk_packages` now raises ValueError if path is a string
+  object. Previously an empty list was returned. (Contributed by Sanyam
+  Khurana in :issue:`24744`.)


'object' is redundant here. Also, mark up "path" as an argument by surrounding it with asterisks.

bitdancer · 2017-06-06T17:08:34Z

 -------

+- bpo-24744: pkgutil.walk_packages function raises ValueError if path is of
+  str object type. Patch by Sanyam Khurana.


... function now raises a ValueError if *path* is a string ...

CuriousLearner · 2017-06-13T17:38:31Z

Thanks @bitdancer :)

vedgar · 2018-01-31T13:51:26Z

That the line is blurry doesn't mean it should be blurried further in the cases where it's obvious what should be raised.

'path should be either *None* or a *list* of paths to look for modules in.'
>>> ValueError.__doc__
'Inappropriate argument value (of correct type).'
>>> TypeError.__doc__
'Inappropriate argument type.'

May I ask what's the rationale for ValueError?

bpo-24744: Raises error in pkgutil.walk_packages if path is str

0b63c15

the-knights-who-say-ni added the CLA signed label Jun 3, 2017

bpo-24744: Add bad input test for pkgutil.walk_packages() function

01d03ed

bitdancer requested changes Jun 4, 2017

View reviewed changes

bpo-24744: Address review comments

675d644

bitdancer approved these changes Jun 5, 2017

View reviewed changes

CuriousLearner added 2 commits June 6, 2017 10:20

bpo-24744: Address review comments in pkgutil tests

b3f8f62

bpo-24744: Fixes docstring of test_walkpackages_on_bad_input method

9fb5eac

bpo-24744: Address review comments

d9dd7a3

bpo-24744: Adds entry in Whatsnew/3.7.rst and NEWS section

082449d

bitdancer requested changes Jun 6, 2017

View reviewed changes

CuriousLearner added 2 commits June 6, 2017 21:10

bpo-24744: Adds entry in Whatsnew/3.7.rst and NEWS section

626599b

bpo-24744: Minor fix in Misc/NEWS

e61e323

bitdancer requested changes Jun 6, 2017

View reviewed changes

CuriousLearner force-pushed the fix-Issue24744 branch from 94637e5 to e61e323 Compare June 6, 2017 17:47

bpo-24744: Minor fix in NEWS and whatsnew/3.7.rst

bb26556

bitdancer approved these changes Jun 6, 2017

View reviewed changes

zware and others added 3 commits June 7, 2017 11:55

Merge branch 'master' into fix-Issue24744

a36bbfa

bpo-24744: Merge branch 'master' & resolves merge conflicts

a8876c1

Merge branch 'master' into fix-Issue24744

5ba7c6b

bitdancer merged commit b9c3da5 into python:master Jun 13, 2017

Uh oh!

Conversation

CuriousLearner commented Jun 3, 2017

Uh oh!

mention-bot commented Jun 3, 2017

Uh oh!

bitdancer commented Jun 3, 2017

Uh oh!

CuriousLearner commented Jun 3, 2017

Uh oh!

bitdancer commented Jun 3, 2017

Uh oh!

CuriousLearner commented Jun 3, 2017

Uh oh!

CuriousLearner commented Jun 3, 2017

Uh oh!

ncoghlan commented Jun 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitdancer commented Jun 6, 2017

Uh oh!

CuriousLearner commented Jun 6, 2017

Uh oh!

bitdancer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CuriousLearner commented Jun 13, 2017

Uh oh!

vedgar commented Jan 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants