bpo-33254: do not return an empty list when asking for the contents of a namespace package #6467

brettcannon · 2018-04-13T22:13:28Z

While the iteration will still be empty and you would only notice the accidental inclusion of the empty list if you introspected on StopIteration's value attribute, it wasn't necessary.

https://bugs.python.org/issue33254

warsaw

LGTM! Thanks and I'll backport this to importlib_resources

serhiy-storchaka

Why yield from? Wouldn't returning the iterator itself be simpler?

And the docstring doesn't match the behavior. It mentions the list of entries.

warsaw · 2018-04-19T00:43:54Z

@serhiy-storchaka Good catch on the docstring. @brettcannon that does need to be updated.

As for the yield from do you mean the one that yields from os.listdir()? That has to yield from since os.listdir() returns a concrete list object.

serhiy-storchaka · 2018-04-19T05:05:42Z

It can return an iterator. And str() is not needed since listdir() supports the path protocol.

return iter(os.listdir(package_directory))

brettcannon · 2018-04-19T16:22:48Z

We can also change the type to Iterable if we wanted.

…rable instead of an iterator

brettcannon · 2018-04-20T18:48:54Z

@warsaw @serhiy-storchaka made everything return an iterable. PTAL.

serhiy-storchaka · 2018-04-22T15:25:28Z

Changing the type to Iterable can break a user code that just calls next() on the result. Don't know whether this is important.

serhiy-storchaka · 2018-04-22T15:27:26Z

Ah since the class is added in 3.7 I don't expect this change will break a lot of user code. 😉

serhiy-storchaka

Yet few suggestions. Mostly minor, but the note about _normalize_path() may be important.

serhiy-storchaka · 2018-04-22T15:29:37Z

Lib/importlib/abc.py

-        """Return an iterator of strings over the contents of the package."""
-        return iter([])
+        """Return an iterable of strings over the contents of the package."""
+        return []


Maybe return a tuple? Creating and iterating a tuple is a tiny bit faster, and the empty tuple consumes less memory.

+0 - I leave it up to @brettcannon

serhiy-storchaka · 2018-04-22T15:30:52Z

Lib/importlib/resources.py

    # Is the package a namespace package?  By definition, namespace packages
    # cannot have resources.  We could use _check_location() and catch the
    # exception, but that's extra work, so just inline the check.
-    if package.__spec__.origin is None or not package.__spec__.has_location:
+    elif package.__spec__.origin is None or not package.__spec__.has_location:
        return []


The same as above. A tuple can be returned here.

serhiy-storchaka · 2018-04-22T15:31:36Z

Lib/importlib/resources.py

-    yield from os.listdir(str(package_directory))
+    else:
+        package_directory = Path(package.__spec__.origin).parent
+        return os.listdir(str(package_directory))


str() is not needed here. os.listdir() supports the path protocol.

str() can be removed also in _normalize_path(). Actually calling str() in _normalize_path() can mask an error when wrong type is passed.

serhiy-storchaka · 2018-04-22T15:40:30Z

Lib/test/test_importlib/test_resource.py

-        self.assertEqual(len(contents), 0)
+    def test_namespaces_cannot_have_resources(self):
+        contents = resources.contents('test.test_importlib.data03.namespace')
+        self.assertEqual(len(set(contents)), 0)


self.assertEqual(list(contents), []) or self.assertFalse(list(contents)) could provide more informative error report (including the content list itself) if failed.

Why set() is used in other tests? Only because the order is not specified? assertCountEqual() in addition could check that items are not repeated (set() hides duplications).

That's a good point. I don't feel a burning need to change all the tests though. One thing that would be helpful is to keep in mind that I am cross-porting changes here into importlib_resources. That's not a reason not to use whatever Python 3.7 constructs make sense, just to be mindful that I have to somehow make it work in 2.7, and 3.4-3.6 :)

warsaw · 2018-04-22T18:32:56Z

FWIW, I'm also not concerned about minor API breakages in importlib_resources. There's a reason why its version number is < 1.0 :)

brettcannon · 2018-04-27T18:51:25Z

OK, I think I addressed all of the comments from @serhiy-storchaka . PTAL.

miss-islington · 2018-04-30T18:31:48Z

Thanks @brettcannon for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

…f a namespace package (pythonGH-6467) (cherry picked from commit 3ab9365) Co-authored-by: Brett Cannon <brettcannon@users.noreply.github.com>

bedevere-bot · 2018-04-30T18:32:55Z

GH-6664 is a backport of this pull request to the 3.7 branch.

…f a namespace package (GH-6467) (cherry picked from commit 3ab9365) Co-authored-by: Brett Cannon <brettcannon@users.noreply.github.com>

brettcannon added 3 commits April 13, 2018 15:06

Tweak a test name

d6d1bca

Update a test to be more strict

737f008

Don't return an empty list in the generator

bc5d2c8

brettcannon added the needs backport to 3.7 label Apr 13, 2018

the-knights-who-say-ni added the CLA signed label Apr 13, 2018

bedevere-bot added the awaiting merge label Apr 13, 2018

Add a news entry

4838af8

brettcannon requested a review from warsaw April 13, 2018 22:15

warsaw approved these changes Apr 14, 2018

View reviewed changes

serhiy-storchaka reviewed Apr 14, 2018

View reviewed changes

brettcannon added 3 commits April 20, 2018 11:07

Merge branch 'master' into resource-empty-list-yield

e9ba597

Move importlib.resources.contents() and related APIs to return an ite…

ceb5510

…rable instead of an iterator

Fix news entry

f124be1

serhiy-storchaka reviewed Apr 22, 2018

View reviewed changes

brettcannon self-assigned this Apr 23, 2018

brettcannon added 3 commits April 27, 2018 11:43

Merge branch 'master' into resource-empty-list-yield

84b52b2

Don't needlessly call str()

b0e5915

Simplify a test

3aae0f6

serhiy-storchaka approved these changes Apr 28, 2018

View reviewed changes

brettcannon merged commit 3ab9365 into python:master Apr 30, 2018

bedevere-bot removed the awaiting merge label Apr 30, 2018

brettcannon deleted the resource-empty-list-yield branch April 30, 2018 18:31

bedevere-bot removed the needs backport to 3.7 label Apr 30, 2018

Uh oh!

bpo-33254: do not return an empty list when asking for the contents of a namespace package #6467

bpo-33254: do not return an empty list when asking for the contents of a namespace package #6467

Uh oh!

Conversation

brettcannon commented Apr 13, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

warsaw left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

warsaw commented Apr 19, 2018

Uh oh!

serhiy-storchaka commented Apr 19, 2018

Uh oh!

brettcannon commented Apr 19, 2018

Uh oh!

brettcannon commented Apr 20, 2018

Uh oh!

serhiy-storchaka commented Apr 22, 2018

Uh oh!

serhiy-storchaka commented Apr 22, 2018

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Apr 22, 2018

Choose a reason for hiding this comment

Uh oh!

warsaw Apr 22, 2018

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Apr 22, 2018

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Apr 22, 2018

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Apr 22, 2018

Choose a reason for hiding this comment

Uh oh!

warsaw Apr 22, 2018

Choose a reason for hiding this comment

Uh oh!

warsaw commented Apr 22, 2018

Uh oh!

brettcannon commented Apr 27, 2018

Uh oh!

miss-islington commented Apr 30, 2018

Uh oh!

bedevere-bot commented Apr 30, 2018

Uh oh!

Uh oh!

brettcannon commented Apr 13, 2018 •

edited by bedevere-bot

Loading