-
Notifications
You must be signed in to change notification settings - Fork 95
feat: provide and use Python version support check #832
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
Changes from 1 commit
6316d70
a136ad7
45cd647
25225fa
1d567c0
f1dbbb4
e3fd56f
8b1dfb1
4b9208e
5cf8652
7d8b1c7
cda8e27
db92fac
118a7c8
f1c46aa
474ec0d
2083b49
4ea124e
c5949c4
6fc1473
8829e20
bdb6260
7896664
54a5611
2e2990b
35a2074
cd64832
e36414a
b6245ca
3a897ba
814ea63
8dee04b
95f4777
a34ec15
a04e2e6
8b3f337
29896cf
d421f98
ac76f7a
e093a96
187f848
b575a16
ecb7211
3c587cf
fbe0f0b
28b0b32
21c9aec
e24c13d
dbc3a26
d472bae
0550f83
491b695
6338389
05bcf6f
fc224b7
a6c40ce
d0414b2
835df53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,18 +16,23 @@ | |
|
|
||
| import warnings | ||
| import sys | ||
| from typing import Optional, Tuple | ||
| from typing import Optional | ||
|
|
||
| from collections import namedtuple | ||
|
|
||
| from ._python_version_support import ( | ||
| _flatten_message, | ||
| _get_distribution_and_import_packages, | ||
| ) | ||
|
|
||
| from packaging.version import parse as parse_version, Version as PackagingVersion | ||
| from packaging.version import parse as parse_version | ||
|
|
||
| DependencyVersion = namedtuple("DependencyVersion", ["version", "version_string"]) | ||
|
|
||
|
|
||
| def get_dependency_version( | ||
| dependency_name: str, | ||
| ) -> Tuple[Optional[PackagingVersion], str]: | ||
| ) -> DependencyVersion: | ||
| """Get the parsed version of an installed package dependency. | ||
|
|
||
| This function checks for an installed package and returns its version | ||
|
|
@@ -38,16 +43,16 @@ def get_dependency_version( | |
| dependency_name: The distribution name of the package (e.g., 'requests'). | ||
|
|
||
| Returns: | ||
| A tuple containing the `packaging.version.Version` object and the | ||
| version string, or `(None, '--')` if the package is not found or | ||
| another error occurs during version discovery. | ||
| A DependencyVersion namedtuple with `version` and `version_string` | ||
| attributes, or `DependencyVersion(None, '--')` if the package is not | ||
| found or another error occurs during version discovery. | ||
| """ | ||
| try: | ||
| if sys.version_info >= (3, 8): | ||
| from importlib import metadata | ||
|
|
||
| version_string = metadata.version(dependency_name) | ||
| return (parse_version(version_string), version_string) | ||
| return DependencyVersion(parse_version(version_string), version_string) | ||
|
|
||
| # TODO(https://github.com/googleapis/python-api-core/issues/835): Remove | ||
| # this code path once we drop support for Python 3.7 | ||
|
|
@@ -56,10 +61,10 @@ def get_dependency_version( | |
| import pkg_resources | ||
|
|
||
| version_string = pkg_resources.get_distribution(dependency_name).version | ||
| return (parse_version(version_string), version_string) | ||
| return DependencyVersion(parse_version(version_string), version_string) | ||
|
|
||
| except Exception: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we know what exceptions to expect? Or does this need to be broad? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any exception related that comes up trying to find the version. I could try to track them down, but at the end of the day I aim for the function to return without an error, but indicating if the version could not be found. So I lean towards catching broadly, but I'm open to counterarguments. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I think that makes sense to be extra cautious here |
||
| return (None, "--") | ||
| return DependencyVersion(None, "--") | ||
|
|
||
|
|
||
| def warn_deprecation_for_versions_less_than( | ||
|
|
@@ -107,12 +112,10 @@ def warn_deprecation_for_versions_less_than( | |
| or not next_supported_version | ||
| ): # pragma: NO COVER | ||
| return | ||
| (version_used, version_used_string) = get_dependency_version( | ||
| dependency_import_package | ||
| ) | ||
| if not version_used: | ||
| dependency_version = get_dependency_version(dependency_import_package) | ||
| if not dependency_version.version: | ||
| return | ||
| if version_used < parse_version(next_supported_version): | ||
| if dependency_version.version < parse_version(next_supported_version): | ||
| ( | ||
| dependency_package, | ||
| dependency_distribution_package, | ||
|
|
@@ -151,8 +154,8 @@ def warn_deprecation_for_versions_less_than( | |
| dependent_package=dependent_package, | ||
| next_supported_version=next_supported_version, | ||
| recommendation=recommendation, | ||
| version_used=version_used, | ||
| version_used_string=version_used_string, | ||
| version_used=dependency_version.version, | ||
| version_used_string=dependency_version.version_string, | ||
| ), | ||
| FutureWarning, | ||
| ) | ||
|
|
||
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.
It seems that this package is missing defining packaging as a explicit dependency.
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.
Also just now experiencing an error on this:
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.
I see that there is already an issue created #848