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

Use importlib.metadata from the standard library on Python 3.10 #772

Closed
wants to merge 3 commits into from

Conversation

encukou
Copy link

@encukou encukou commented Jun 24, 2021

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.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

python: [3.6, 3.7, 3.8, 3.9]

envlist = lint,types,py{36,37,38,39},integration,docs

Comment on lines +18 to +23
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 version

That feels a little exotic, but so does using importlib_metadata in the first place (even though it was for a good reason, IIRC).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're free to do that, of course!

@encukou
Copy link
Author

encukou commented Jun 25, 2021

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

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.

@bhrutledge
Copy link
Contributor

Closing this in favor of #773. Thanks for taking a pass at this, @encukou; I think it was a worthwhile discussion.

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.

None yet

3 participants