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

gh-128862: use importlib.resources to acquire doctest resources #128865

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

graingert
Copy link
Contributor

@graingert graingert commented Jan 15, 2025

@graingert graingert requested a review from brettcannon January 15, 2025 13:42
Lib/doctest.py Outdated Show resolved Hide resolved
@graingert graingert requested a review from jaraco January 26, 2025 09:35
@jaraco jaraco self-assigned this Feb 2, 2025
Comment on lines +242 to +261
try:
loader = package.__loader__
except AttributeError:
pass
else:
if loader is not None:
return (
importlib.resources.read_text(package, filename, encoding=encoding),
filename,
)

try:
package.__spec__.loader
except AttributeError:
pass
else:
return (
importlib.resources.read_text(package, filename, encoding=encoding),
filename,
)
Copy link
Member

@AA-Turner AA-Turner Feb 2, 2025

Choose a reason for hiding this comment

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

mod.__loader__ is deprecated in favour of mod.__spec__.loader, should we switch the order here? It may also be worth factoring the logic to a helper function.

Comment on lines +242 to +261
try:
loader = package.__loader__
except AttributeError:
pass
else:
if loader is not None:
return (
importlib.resources.read_text(package, filename, encoding=encoding),
filename,
)

try:
package.__spec__.loader
except AttributeError:
pass
else:
return (
importlib.resources.read_text(package, filename, encoding=encoding),
filename,
)
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking the duplication here is unnecessary. In fact, I can't tell from reading the code why there are two paths that are checked before invoking the same behavior. Why not instead:

Suggested change
try:
loader = package.__loader__
except AttributeError:
pass
else:
if loader is not None:
return (
importlib.resources.read_text(package, filename, encoding=encoding),
filename,
)
try:
package.__spec__.loader
except AttributeError:
pass
else:
return (
importlib.resources.read_text(package, filename, encoding=encoding),
filename,
)
with contextlib.suppress(Exception):
return (
importlib.resources.read_text(package, filename, encoding=encoding),
filename,
)

Copy link
Member

Choose a reason for hiding this comment

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

My feeling is that doctest shouldn't be concerning itself with the implementation details of whether a module should have loadable resources or not, but should be able to rely on importlib.resources to assert that state.

In https://github.com/jaraco/cpython/tree/gh-128862/importlib-resources-load, I've first refactored the code to attempt to disentangle concerns. I've then trapped AttributeError (what importlib.resources happens to throw) when loading the resource. I considered trapping Exception here, but I'm worried that might mask other unexpected conditions.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants