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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51d5c18
Use importlib.metadata from the standard library on Python 3.10
encukou Jun 24, 2021
40d2e7d
Add a changelog entry
encukou Jun 24, 2021
002eb85
Use from-import for `importlib[_.]metadata`
encukou Jun 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/772.mics.rst
View file
Open in desktop
@@ -0,0 +1 @@
Remove dependency on external importlib_metadata on Python 3.10.
2 changes: 1 addition & 1 deletion setup.cfg
View file
Open in desktop
Expand Up @@ -40,7 +40,7 @@ install_requires=
requests >= 2.20
requests-toolbelt >= 0.8.0, != 0.9.0
tqdm >= 4.14
importlib_metadata >= 3.6
importlib_metadata >= 3.6; python_version<"3.10"
keyring >= 15.1
rfc3986 >= 1.4.0
colorama >= 0.4.3
Expand Down
9 changes: 7 additions & 2 deletions twine/__init__.py
View file
Open in desktop
Expand Up @@ -24,9 +24,14 @@

__copyright__ = "Copyright 2019 Donald Stufft and individual contributors"

import importlib_metadata
import sys

metadata = importlib_metadata.metadata("twine")
if sys.version_info[:2] >= (3, 10):
from importlib.metadata import metadata as _metadata
else:
from importlib_metadata import metadata as _metadata

metadata = _metadata("twine")


__title__ = metadata["name"]
Expand Down
9 changes: 7 additions & 2 deletions twine/cli.py
View file
Open in desktop
Expand Up @@ -12,10 +12,15 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import argparse
import sys
from typing import Any, List, Tuple

from importlib_metadata import entry_points
from importlib_metadata import version
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
Comment on lines +18 to +23
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!


import twine

Expand Down
7 changes: 6 additions & 1 deletion twine/package.py
View file
Open in desktop
Expand Up @@ -16,9 +16,14 @@
import os
import re
import subprocess
import sys
from typing import Dict, NamedTuple, Optional, Sequence, Tuple, Union

import importlib_metadata
if sys.version_info[:2] >= (3, 10):
import importlib.metadata as importlib_metadata
else:
import importlib_metadata

import pkginfo

from twine import exceptions
Expand Down