-
Notifications
You must be signed in to change notification settings - Fork 416
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
Relax VenvInspectInformation attribute typing #1087
Relax VenvInspectInformation attribute typing #1087
Conversation
Use Iterable as a more generic type hint. * Import Iterable from typing * Replace all the corresponding type hints * Swap the explicit list() call to a tuple() call since we are not modifying the `VenvInspectInformation.distributions` Resolves: pypa#629
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your description, and it looks good to me generally. 👍
I would prefer Collection here, since Iterable sort of hints you are only expected to interate through this at most once (which is not the case). |
Thank you for your feedback @uranusjr 👍 The last commit reflects this change. |
Looks like some tests are failed with this error: |
As described here |
Doing a |
I added |
Thanks! |
Description
This PR aims to remove the explicit call to
list()
made invenv_inspect.py
by using the type thatimportlib.metadata.distributions()
returns (anIterator
fromitertools.chain.from_iterable
, see) from wrappingDistribution.discover(**kwargs)
.However, the code logic would be flawed if we used an
Iterator
(in this case skipping the explicitlist()
call) because we could only iterate on it once and need to do it multiple times in_dfs_package_apps
to retrieve the dependencies for a given package correctly.Therefore, an
Iterable
was used as a more generic type hint for more flexibility, andtuple
was used instead oflist
since we are not modifyingVenvInspectInformation.distributions
.docs/changelog.md
Summary of changes
Test plan
Tested by running
Resolves: #629