Use importlib.metadata from the standard library on Python 3.10#772
Use importlib.metadata from the standard library on Python 3.10#772
Conversation
According to the [importlib_metadata readme], all features from 4.4 were merged to Python 3.10. So, twine can have one less external dependency on the upcoming Python release. [importlib_metadata readme]: https://pypi.org/project/importlib-metadata/
| import sys | ||
|
|
||
| if sys.version_info[:2] >= (3, 10): | ||
| from importlib import metadata as importlib_metadata |
There was a problem hiding this comment.
This is different from below and makes grep'ping for the standard library harder for the next time we have to rely only one the backport library. Please be consistent here so in a few months we can more easily revert these changes. (I'm guessing here based on the patterns we've seen.)
There was a problem hiding this comment.
I would suggest pushing the boundary to (3, 11) instead of reverting. As I understand it, importlib_metadata is always meant to be there temporarily, until stdlib catches up.
(But of course, I'm only suggesting – it's your project!)
There was a problem hiding this comment.
Thanks for the effort here, @encukou. I'm generally supportive of reducing dependencies, but I'm not sure about this change. It seems like Twine will be dependent on importlib_metadata for a long time (i.e., as long as it supports versions less that 3.10). Also, I'm assuming that importlib_metadata will continue to work in 3.10. With that in mind, I'm not sure the added complexity is worth it. That said, I'm open to other maintainers' opinions (esp. @jaraco, who maintains importlib_metadata).
Also, I'd probably wait until 3.10 is actually released, and Twine has stated support for it. So, I think a precursor to this would be adding 3.10 to our testing and CI configurations:
twine/.github/workflows/main.yml
Line 26 in e07ce0e
Line 3 in e07ce0e
| if sys.version_info[:2] >= (3, 10): | ||
| from importlib.metadata import entry_points | ||
| from importlib.metadata import version | ||
| else: | ||
| from importlib_metadata import entry_points | ||
| from importlib_metadata import version |
There was a problem hiding this comment.
I would prefer to do the version check in one place. Given that it's already being done in twine/__init__.py, I think this could be something like:
from twine.importlib_metadata import entry_points
from twine.importlib_metadata import versionThat feels a little exotic, but so does using importlib_metadata in the first place (even though it was for a good reason, IIRC).
There was a problem hiding this comment.
While that's DRY, it also will add an implicit dependency on that import in twine/__init__.py and every import in that file slows down the start-up speed of twine. Users often complain of how long CLIs take to start and so I'd rather not start a pattern of importing packages that may have been imported in that file because people tend to glom onto that and re-use that pattern needlessly.
If we want somewhere to centralize imports like this, I'd suggest a separate module altogether, e.g., twine/_compat.py where we have this logic and then twine/__init__.py can use that and so can other places
There was a problem hiding this comment.
Since __init__ already imports importlib.metadata for its own use, the argument is only that people might generalize it too much, right?
Should I do this but add a comment explaining it's not a general pattern?
There was a problem hiding this comment.
Thanks for the explanation, @sigmavirus24. I like the twine/_compat.py soluton.
However, after reading through PR that added standardized on importlib_metadata, specifically #732 (comment), I'm still hesitant to accept the complexity this introduces for what feels like a small benefit.
@encukou What do you think about closing this PR, and opening an issue so that references it, so that we can come back to it after the release of 3.10 and/or 3.11?
There was a problem hiding this comment.
You're free to do that, of course!
Fair, though I assumed there's some value in doing preparations before the 3.10 release. Even if they're only tested manually/outside Twine's CI. |
3 participants
According to the importlib_metadata readme, all features from 4.4
were merged to Python 3.10. So, twine can have one less external
dependency on the upcoming Python release.