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.resources to load files #5896

Closed

Conversation

Illviljan
Copy link
Contributor

@Illviljan Illviljan commented Oct 25, 2021

Following: https://importlib-resources.readthedocs.io/en/latest/migration.html#pkg-resources-resource-string

  • Closes #xxxx
  • Tests added
  • Passes pre-commit run --all-files
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

@github-actions
Copy link
Contributor

Unit Test Results

         6 files           6 suites   56m 51s ⏱️
16 209 tests 14 462 ✔️ 1 736 💤 11
90 450 runs  82 237 ✔️ 8 180 💤 33

For more details on these failures, see this check.

Results for commit 98dec8e.

Copy link
Collaborator

@keewis keewis left a comment

Choose a reason for hiding this comment

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

this partially overlaps with #5845, but as importlib.resources is also in python=3.7 we can merge this PR now, while #5845 probably needs to wait until we can require python>=3.8

@@ -15,7 +14,7 @@
def _load_static_files():
"""Lazily load the resource files into memory the first time they are needed"""
return [
pkg_resources.resource_string("xarray", fname).decode("utf8")
resources.files("xarray").joinpath(fname).read_bytes().decode("utf8")
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason why you don't use read_text? Otherwise I'd use this:

Suggested change
resources.files("xarray").joinpath(fname).read_bytes().decode("utf8")
importlib.resources.read_text("xarray", fname, encoding="utf-8")

which is probably the same as

Suggested change
resources.files("xarray").joinpath(fname).read_bytes().decode("utf8")
resources.files("xarray").joinpath(fname).read_text(encoding="utf-8")

Copy link
Contributor Author

@Illviljan Illviljan Oct 25, 2021

Choose a reason for hiding this comment

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

It was the recommended replacement method, https://importlib-resources.readthedocs.io/en/latest/migration.html#pkg-resources-resource-string

Because it crashes:

[
    resources.read_text("xarray", fname, encoding="utf-8")
    for fname in STATIC_FILES
]
(...)
ValueError: 'static/html/icons-svg-inline.html' must be only a file name

And isn't an exact replacement:

a = [
    pkg_resources.resource_string("xarray", fname).decode("utf8")
    for fname in STATIC_FILES
]

b = [
    resources.files("xarray").joinpath(fname).read_text(encoding="utf-8")  # 3.9 only
    for fname in STATIC_FILES
]

a == b # b is missing \r at least
Out[86]: False

Copy link
Collaborator

@keewis keewis Oct 25, 2021

Choose a reason for hiding this comment

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

right, for py3.9 the replacement is exact (at least for me) but maybe not on other python versions or platforms? I'm guessing read_text will transform newlines to \r\n on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be the platform then, I used windows 10 and python 3.9.6.

Copy link
Collaborator

@keewis keewis Oct 25, 2021

Choose a reason for hiding this comment

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

and I'm on linux with py3.9.

Does a contain \r for you? If so, it might be the newline option to open...

@Illviljan
Copy link
Contributor Author

Illviljan commented Oct 25, 2021

Ah, I missed that #5845 had fixed this as well. I can close this one then. It never is as easy as I think...

@Illviljan Illviljan closed this Oct 25, 2021
@keewis
Copy link
Collaborator

keewis commented Oct 25, 2021

as I said, we could merge this one as soon as it's finished while the other one will definitely have to wait until end of December (unless conda changed the way the preprocessor tags work since I last looked at it).

This really depends on when whether we want to drop pkg_resources in a single PR or multiple, but personally I'd say it's fine to do a partial conversion.

@Illviljan Illviljan deleted the remove_pkg_resources.resource_string branch August 12, 2022 09:05
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.

2 participants