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.

Sign up for GitHub

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

Jump to bottom

importlib.readers not valid until python 3.10 #9609

Merged
merged 6 commits into from Feb 8, 2022
Merged

importlib.readers not valid until python 3.10 #9609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
15ddccf
importlib.readers not valid until python 3.10
kdelee Feb 4, 2022
5f1a2f3
Fix invalid importing of `importlib.reader` in Python 3.9
nicoddemus Feb 4, 2022
66dc79e
Update changelog/9608.bugfix.rst
nicoddemus Feb 4, 2022
f86a87a
Update src/_pytest/assertion/rewrite.py
nicoddemus Feb 4, 2022
d45a19c
move function specific imports to function
kdelee Feb 7, 2022
2073cce
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/9608.bugfix.rst
View file
Open in desktop
@@ -0,0 +1 @@
Fix invalid importing of ``importlib.readers`` in Python 3.9.
10 changes: 6 additions & 4 deletions src/_pytest/assertion/rewrite.py
View file
Open in desktop
Expand Up @@ -273,13 +273,15 @@ def get_data(self, pathname: Union[str, bytes]) -> bytes:
with open(pathname, "rb") as f:
return f.read()

if sys.version_info >= (3, 9):
if sys.version_info >= (3, 10):
Copy link
Member

Choose a reason for hiding this comment

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

I agree that this should be changed to (3, 10) since FileReader isn't needed there


def get_resource_reader(self, name: str) -> importlib.abc.TraversableResources: # type: ignore
Copy link

Choose a reason for hiding this comment

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

Hey, I'm currently writing a code analysis tool for Python projects. It pointed me to this code part, which I find quite interesting. Hence, I would have some questions about it:

  1. Where exactly is the get_resource_reader() function called? I can't find any references to it.
  2. Why is the function defined by a condition? Would it make sense to move the version check inside the function with an early return? (As there is already a version check within it) As the function is public, it would keep the interface of the AssertionRewritingHook independent to the Python version.

I think these questions may be adressed to @kdelee or @nicoddemus? I would be really happy to hear from you as this would give me some qualitative insights on the reports of my tool.

Kind regards,
Matthias

Copy link
Member

Choose a reason for hiding this comment

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

it's called by the python import machinery itself. this function isn't valid until 3.10 (it breaks 3.9)

Copy link

Choose a reason for hiding this comment

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

I saw the Github issue on the FileReader import causing the break for 3.9

Before, the import statement in the function was without any version check. But having the check sys.version_info >= (3, 10) within the function with an early return shouldn't break anything as the import statement would never be reached for 3.9.
Or am I overlooking anything why the function would be invalid otherwise?

Copy link
Member

Choose a reason for hiding this comment

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

there's backports of parts of importlib which would reach into this and if the function is defined try and call it (and then error) -- by not defining the function, those backports won't even attempt to call it and therefore won't crash

Copy link

Choose a reason for hiding this comment

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

Ahh ok I understand. Thanky you very much for your explanation, this helps me a lot!

from types import SimpleNamespace
from importlib.readers import FileReader
if sys.version_info < (3, 11):
from importlib.readers import FileReader
else:
from importlib.resources.readers import FileReader

return FileReader(SimpleNamespace(path=self._rewritten_names[name]))
return FileReader(types.SimpleNamespace(path=self._rewritten_names[name]))


def _write_pyc_fp(
Expand Down