bpo-33254: do not return an empty list when asking for the contents of a namespace package by brettcannon · Pull Request #6467 · 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
Conversation19 Commits10 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 }})
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
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks and I'll backport this to importlib_resources
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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.
It can return an iterator. And str() is not needed since listdir() supports the path protocol.
return iter(os.listdir(package_directory))
We can also change the type to Iterable if we wanted.
…rable instead of an iterator
Changing the type to Iterable can break a user code that just calls next() on the result. Don't know whether this is important.
Ah since the class is added in 3.7 I don't expect this change will break a lot of user code. 😉
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yet few suggestions. Mostly minor, but the note about _normalize_path() may be important.
| """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 [] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe return a tuple? Creating and iterating a tuple is a tiny bit faster, and the empty tuple consumes less memory.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # 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 [] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as above. A tuple can be returned here.
| yield from os.listdir(str(package_directory)) |
|---|
| else: |
| package_directory = Path(package.__spec__.origin).parent |
| return os.listdir(str(package_directory)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
FWIW, I'm also not concerned about minor API breakages in importlib_resources. There's a reason why its version number is < 1.0 :)
Thanks @brettcannon for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
…f a namespace package (pythonGH-6467)
(cherry picked from commit 3ab9365)
Co-authored-by: Brett Cannon brettcannon@users.noreply.github.com
miss-islington added a commit that referenced this pull request
…f a namespace package (GH-6467)
(cherry picked from commit 3ab9365)
Co-authored-by: Brett Cannon brettcannon@users.noreply.github.com