Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
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 ourterms of service andprivacy 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?
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 asuppress context manager.
Perhaps:
text=Noneifmodule_relative:package=_normalize_module(package,depth=3)try:file=importlib.resources.files(package)/filenametext=file.read_text(encoding=encoding)exceptAttributeError:filename=_module_relative_path(package,filename)iftextisNone:withopen(filename,encoding=encoding)asf:text=f.read()returntext,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 fromException toAttributeError, 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.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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!
Misc/NEWS.d/next/Library/2025-01-15-11-57-42.gh-issue-128862.TDHQdR.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| 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 byimportlib.resources.files() or byfile.read_text()? Cantext be None?
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Uh oh!
There was an error while loading.Please reload this page.
doctest._load_testfileuses deprecated Loader.get_data API, it should use importlib.resources #128862