pypa / setuptools

Official project repository for the Setuptools build system
https://pypi.org/project/setuptools/
MIT License
2.35k stars 1.15k forks source link

Make `pkg_resources.find_nothing` return an (empty) `Generator` of `Distribution` #4249

Closed Avasam closed 2 months ago

Avasam commented 3 months ago

Summary of changes

This aligns with find_eggs_in_zip and find_on_path and ensures find_distributions always returns a generator.

This was noticed when attempting to type the pkg_resoruces package for #2345

xref https://github.com/python/typeshed/pull/11512#discussion_r1508147634

Pull Request Checklist

[PR docs]: https://setuptools.pypa.io/en/latest/development/developer-guide.html#making-a-pull-request

abravalheri commented 2 months ago

Hi @Avasam, would it make more sense that instead of changing the implementation we just consider that the distribution_finder in the register_finder function returns as type an Iterable instead of Generator?

If we consider the implementation, that seems to be the minimum requirement for the functions to work, isn't it? It is also more generic... So to me it makes sense to not raise the bar in terms of type specs.

The documentation probably uses the term yield with a more general way, rather than to direct imply the use of the yield keyword in Python.

Avasam commented 2 months ago

@abravalheri Yes it makes perfect sense. In fact going with Iterable is what I did in that typeshed PR comment I linked.

I'd want to update the docstrings as well because "yield" is a bit misleading in Python given it has a special meaning (a meaning that is true most of the time, except in that special scenario). But if you want me to keep the docstrings as-is I can do so, my needs only really concern the type annotations.

abravalheri commented 2 months ago

@Avasam would something like the following be an appropriate documentation change?

diff --git i/pkg_resources/__init__.py w/pkg_resources/__init__.py
index 625d1f83d..8ee988114 100644
--- i/pkg_resources/__init__.py
+++ w/pkg_resources/__init__.py
@@ -2049,7 +2049,7 @@ def register_finder(importer_type, distribution_finder):

     `importer_type` is the type or class of a PEP 302 "Importer" (sys.path item
     handler), and `distribution_finder` is a callable that, passed a path
-    item and the importer instance, yields ``Distribution`` instances found on
+    item and the importer instance, iterates over ``Distribution`` instances found on
     that path item.  See ``pkg_resources.find_on_path`` for an example."""
     _distribution_finders[importer_type] = distribution_finder
Avasam commented 2 months ago

@Avasam would something like the following be an appropriate documentation change?

diff --git i/pkg_resources/__init__.py w/pkg_resources/__init__.py
index 625d1f83d..8ee988114 100644
--- i/pkg_resources/__init__.py
+++ w/pkg_resources/__init__.py
@@ -2049,7 +2049,7 @@ def register_finder(importer_type, distribution_finder):

     `importer_type` is the type or class of a PEP 302 "Importer" (sys.path item
     handler), and `distribution_finder` is a callable that, passed a path
-    item and the importer instance, yields ``Distribution`` instances found on
+    item and the importer instance, iterates over ``Distribution`` instances found on
     that path item.  See ``pkg_resources.find_on_path`` for an example."""
     _distribution_finders[importer_type] = distribution_finder

That's still not quite right, I'd say it "returns an Iterable of Distribution" rather than "iterates over Distribution"

Avasam commented 2 months ago

Since the entire premise of this PR is wrong, and that the only change is docstring anyway. I'll avoid doing a ship of Theseus out of it and I'll just finish typing the register_* methods with the established Iterable return type for distribution finders. After #4246 is merged.