Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented Feb 29, 2024

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 python/typeshed#11512 (comment)

Pull Request Checklist

@Avasam Avasam changed the title Make find_nothing return an (empty) Generator of Distribution Make pkg_resources.find_nothing return an (empty) Generator of Distribution Feb 29, 2024
@Avasam Avasam force-pushed the pkg_resources-find_nothing-Generator branch 2 times, most recently from 7e89c92 to 59a7376 Compare February 29, 2024 20:58
This aligns with `find_eggs_in_zip` and `find_on_path` and ensures `find_distributions` always returns a generator
@Avasam Avasam force-pushed the pkg_resources-find_nothing-Generator branch from 59a7376 to bc05c46 Compare February 29, 2024 21:03
@abravalheri
Copy link
Contributor

abravalheri commented Mar 5, 2024

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
Copy link
Contributor Author

Avasam commented Mar 5, 2024

@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
Copy link
Contributor

@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
Copy link
Contributor Author

Avasam commented Mar 6, 2024

@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
Copy link
Contributor Author

Avasam commented Mar 6, 2024

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.

@Avasam Avasam closed this Mar 6, 2024
@Avasam Avasam deleted the pkg_resources-find_nothing-Generator branch May 9, 2024 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants