-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Implement PEP 302 optional get_code loader method #13315
base: main
Are you sure you want to change the base?
Conversation
We implement the optional get_code loader method. This increases compatibility with other tools/libraries that need to manipulate the code object of a module before it is executed.
for more information, see https://pre-commit.ci
src/_pytest/assertion/rewrite.py
Outdated
@@ -126,7 +127,7 @@ def find_spec( | |||
): | |||
return None | |||
else: | |||
fn = spec.origin | |||
self.fn = fn = spec.origin |
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 see no reason add this random state - the spec is used to get it anyway
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 is needed inside get_code
as the argument is the fullname
of the module rather than the file path. The same thing is done in the FileLoader
from the stdlib https://github.com/python/cpython/blob/2433cc79d79d9c1db8e53d4b9bde26e9a47fb0b9/Lib/importlib/_bootstrap_external.py#L924
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.
as the object you changed is not just a loader, but also a MetaPathFinder - it would be necessary to split those objects before the state would be permissible as there would be more than one loader instance with different values
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.
cpython has those split up as well - see https://github.com/python/cpython/blob/2433cc79d79d9c1db8e53d4b9bde26e9a47fb0b9/Lib/importlib/_bootstrap_external.py#L1267 for reference
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 would be necessary to split those objects before the state would be permissible
I could try to create a full name -> origin mapping on the finder instead, to keep the changes small. How does that sound?
unless there is a compelling example where this helps im opposed to the change as it introduces fragile state just to introduce one optional old method |
Here's one concrete example where we have to work around this DataDog/dd-trace-py#12812
I'm not sure I understand the meaning of "old" here 🤔 . I think implementing this method is good courtesy to other libraries that might have the need to interact with the code object. Here pytest is making the assumption that it is the only tool to manipulate code object, and therefore feels no need to implement |
please understand that composing a PR about a import behavior changes in the github ux that adds state that may be fragile in multiple steps wit errors and without providing background for the need creates a bias |
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 seems good for now - i haven't yet taken a look if we have a place in the test-suite that would allow for a reasonably easy to write test for this
We implement the optional get_code loader method. This increases compatibility with other tools/libraries that need to manipulate the code object of a module before it is executed.