Provide the XonshImportEventLoader with all the functions that pkg_re… #3636
Conversation
Achimh3011
force-pushed
the
pkg_resources_missing_methods
branch
from
d1a9749
to
3dc2b75
Compare
There was a problem hiding this comment.
Hi @Achimh3011, thanks for putting this in and sorry for the delay.
I don't really know if that matters, but I'm a bit concerned about getattr/hasattr functionality. The proposed implementation returns True for hasattr(wrapper, "get_data"), but throws AttributeError for actual invocation of the wrapper.get_data(path) method if the underlying loader object does not implement the respective method. Maybe we should also override __getattribute__ to do something like:
def __getattribute__(self, name):
if name in WRAPPED_ATTRIBUTES:
return getattr(self.loader, name)
return object.__getattribute__(self, name)Where WRAPPED_ATTRIBUTES is a list of loader interface method names.
Edit: The above implementation would probably make the wrapper implementations of the interface methods redundant, because the attributes get resolved using __getattribute__ call at runtime. That behavior may, however, be specific to CPython implementation and may not work in general. The wrapped methods would for sure be missing from __dict__ which may or may not matter. Unfortunately, my Python expertise is insufficient for me to tell. @xonsh/xore, any input on that matter?
|
That is definitely a better approach than implementing all these wrapper methods. I'll try it out and modify the PR if it works. My expectation would be that other Python implementations will work with the new approach as well. Maybe it is harder to optimize, but the semantics should hold. |
…sources would expect. Fixes xonsh#3607 Signed-off-by: Achim Herwig <achim.herwig@wodca.de>
Achimh3011
force-pushed
the
pkg_resources_missing_methods
branch
from
3dc2b75
to
9de0303
Compare
xonsh/imphooks.py
Outdated
| def module_repr(self, module): | ||
| """Legacy module repr, provided for backwards compatibility.""" | ||
| return self.loader.module_repr(module) | ||
| def __getattribute__(self, name): |
There was a problem hiding this comment.
Oops! That's my fault. @astronouth7303 is right - __getattr__ is the one we need.
There was a problem hiding this comment.
Argh, right! It worked and I did not think twice...
|
Hi @Achimh3011 - What is the purpose of this exactly? I haven't read the issue this is purporting to fix yet, but it seems like we should avoid |
|
Well, as you can see in #3607, on some platforms xonsh does not start at all, because dependencies use Why this has become a problem only recently (and only on some platforms), I don't know. Prior to this PR, there were two "compat" methods present, now a larger number of methods is supported. As this is all based on dynamic lookup, the PR does not alter the (non-)dependency of xonsh, but ensures xonsh works with dependencies using |
xonsh/imphooks.py
Outdated
| @@ -270,6 +270,24 @@ def find_spec(self, fullname, path, target=None): | |||
| return spec | |||
|
|
|||
|
|
|||
| _XIEVL_WRAPPED_ATTRIBUTES = [ | |||
|
Right, so we go out of our way to make sure that our critical dependencies do not use pkg_resources. I guess I am OK with this, subject to the frozenset comment |
|
Also, this needs a news entry 😉 |
|
Thanks! |
…sources would expect.
It is not clear why they are now missed when it worked for years without them. Comments very welcome, also on the methods thate are redirected - maybe not all are needed / maybe some are missing.
Fixes #3607