-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Use importlib.resources to load files #5896
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
Conversation
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.
@@ -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") |
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.
is there a reason why you don't use read_text
? Otherwise I'd use this:
resources.files("xarray").joinpath(fname).read_bytes().decode("utf8") | |
importlib.resources.read_text("xarray", fname, encoding="utf-8") |
which is probably the same as
resources.files("xarray").joinpath(fname).read_bytes().decode("utf8") | |
resources.files("xarray").joinpath(fname).read_text(encoding="utf-8") |
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 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
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.
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?
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.
Might be the platform then, I used windows 10 and python 3.9.6.
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.
and I'm on linux with py3.9.
Does a
contain \r
for you? If so, it might be the newline
option to open
...
Ah, I missed that #5845 had fixed this as well. I can close this one then. It never is as easy as I think... |
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 |
Following: https://importlib-resources.readthedocs.io/en/latest/migration.html#pkg-resources-resource-string
pre-commit run --all-files
whats-new.rst
api.rst