-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-128862: use importlib.resources to acquire doctest resources #128865
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
base: main
Are you sure you want to change the base?
gh-128862: use importlib.resources to acquire doctest resources #128865
Conversation
Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>
Lib/doctest.py
Outdated
def _load_testfile(filename, package, module_relative, encoding): | ||
if module_relative: | ||
package = _normalize_module(package, 3) | ||
with contextlib.suppress(Exception): |
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.
What exceptions should be suppressed?
Exception
is too wide class. It includes OSError, UnicodeDecodingError, MemoryError, which currently are not suppressed.
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.
This control flow feels odd, because we're returning in a suppress
context manager.
Perhaps:
text = None
if module_relative:
package = _normalize_module(package, depth=3)
try:
file = importlib.resources.files(package) / filename
text = file.read_text(encoding=encoding)
except AttributeError:
filename = _module_relative_path(package, filename)
if text is None:
with open(filename, encoding=encoding) as f:
text = f.read()
return text, filename
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.
cc @graingert, are you able to get to this before Tuesday (feature freeze)?
A
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 think @jaraco has taken over this PR
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've applied my suggestion, narrowing from Exception
to AttributeError
, per Jason above:
Even better would be if importlib.resources could throw a more specific exception when the indicated module is unable to resolve resources, but since it currently just raises an AttributeError, perhaps it's best to just trap that exception, rather than try to pre-emptively detect that exception will occur.
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
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.
Do not suppress arbitrary Exception, suppress only the necessary.
Also, I do not think that it is worth to import contextlib for a simple except.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
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.
thanks!
@@ -0,0 +1 @@ | |||
Use ``importlib.resources`` to acquire test files in ``doctest`` |
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.
Use ``importlib.resources`` to acquire test files in ``doctest`` | |
Use :mod:`importlib.resources` to acquire test files in :mod:`doctest`. |
try: | ||
file = importlib.resources.files(package) / filename | ||
text = file.read_text(encoding=encoding) | ||
except AttributeError: |
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 it raised by importlib.resources.files()
or by file.read_text()
? Can text
be None?
doctest._load_testfile
uses deprecated Loader.get_data API, it should use importlib.resources #128862