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

replace pkg_resources with importlib in ska_helpers.get_version #56

Merged
merged 1 commit into from
Jan 31, 2024
Merged

replace pkg_resources with importlib in ska_helpers.get_version #56

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 11 additions & 26 deletions ska_helpers/version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Licensed under a 3-clause BSD style license - see LICENSE.rst
"""
The ``ska_helpers.version`` module provides utilities to handle package
versions. The version of a package is determined using pkg_resources if it is
versions. The version of a package is determined using importlib if it is
installed, and `setuptools_scm <https://github.com/pypa/setuptools_scm/>`_
otherwise.
"""
Expand All @@ -22,7 +22,7 @@
warnings.filterwarnings(
"ignore", message=r"Module \w+ was already imported", category=UserWarning
)
from pkg_resources import DistributionNotFound, get_distribution
from importlib import resources, metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems fine, though unlike pkg_resources, we're already importing importlib at the top of the file. Does this need to be in this catch_warnings block?

Copy link
Member

Choose a reason for hiding this comment

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

Good point. I'm fighting two instincts, (1) cut any code that doesn't seem necessary (and re-test) and (2) let it go and approve and move on. I'm honestly not sure, but otherwise this PR looks good and we should get it merged one way or the other.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These two sub-modules are not imported by the import statement at the top of the file, so they have to be explicitly imported. This goes to say that the fact that importlib was already imported says nothing about whether the warning would be issued here

I don't know whether importing without the warnings filter would trigger the warning. I do not remember if there were special cases when this was triggered, or if a simple call to testr is enough to check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just removed the filter warning in the master branch, and I did not get this warning, so I do not know in which circumstances this warning was issued to begin with.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine to leave this as is.



def get_version_logger(level_stdout, level_string):
Expand Down Expand Up @@ -93,32 +93,17 @@ def get_version(package, distribution=None):
" Getting distribution "
f"dist_info=get_distribution({distribution or package})"
)
dist_info = get_distribution(distribution or package)
version = dist_info.version
log(f" {dist_info.location=}")
log(f" {dist_info.version=}")

# Check if the imported package __init__.py file has the same location
# as the distribution that was found. If working in a local git repo
# that does not have a <package>.egg-info directory, get_distribution()
# will find an installed version. Windows does not necessarily
# respect the case so downcase everything.
ok = module_file.lower().startswith(dist_info.location.lower())
if ok:
log(" distinfo.location matches module_file")
else:
log(
" WARNING: distinfo.location does not match module_file, "
"falling through to setuptools_scm"
)
assert ok
version = metadata.version(distribution or package)
location = resources.files(distribution or package)
log(f" {location=}")
log(f" {version=}")

# If the dist_info.location appears to be a git repo, then
# get_distribution() has gotten a "local" distribution and the
# dist_info.version just corresponds to whatever version was the
# importlib has gotten a "local" distribution and the
# version just corresponds to whatever version was the
# last run of "setup.py sdist" or "setup.py bdist_wheel", i.e.
# unrelated to current version, so ignore in this case.
git_dir = Path(dist_info.location, ".git")
git_dir = location.parent / ".git"
bad = git_dir.exists() and git_dir.is_dir()
if bad:
log(
Expand All @@ -129,8 +114,8 @@ def get_version(package, distribution=None):
log(" distinfo.location looks OK (not a git repo)")
assert not bad

except (DistributionNotFound, AssertionError):
# Get_distribution failed or found a different package from this
except (metadata.PackageNotFoundError, AssertionError):
# metadata.version failed or found a different package from this
# file, try getting version from source repo.
from setuptools_scm import get_version

Expand Down
Loading