bpo-33254: do not return an empty list when asking for the contents of a namespace package#6467
Conversation
warsaw
left a comment
There was a problem hiding this comment.
LGTM! Thanks and I'll backport this to importlib_resources
serhiy-storchaka
left a comment
There was a problem hiding this comment.
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 |
|
It can return an iterator. And return iter(os.listdir(package_directory)) |
|
We can also change the type to |
|
@warsaw @serhiy-storchaka made everything return an iterable. PTAL. |
|
Changing the type to |
|
Ah since the class is added in 3.7 I don't expect this change will break a lot of user code. 😉 |
serhiy-storchaka
left a comment
There was a problem hiding this comment.
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 [] |
There was a problem hiding this comment.
Maybe return a tuple? Creating and iterating a tuple is a tiny bit faster, and the empty tuple consumes less memory.
| # 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 [] |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 |
|
OK, I think I addressed all of the comments from @serhiy-storchaka . PTAL. |
|
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>
|
GH-6664 is a backport of this pull request to the 3.7 branch. |
While the iteration will still be empty and you would only notice the accidental inclusion of the empty list if you introspected on
StopIteration'svalueattribute, it wasn't necessary.https://bugs.python.org/issue33254